From owner-svn-src-all@FreeBSD.ORG Sun May 4 02:48:48 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id CC2B2507; Sun, 4 May 2014 02:48:48 +0000 (UTC) Received: from mail-we0-x230.google.com (mail-we0-x230.google.com [IPv6:2a00:1450:400c:c03::230]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id EE39A11AE; Sun, 4 May 2014 02:48:47 +0000 (UTC) Received: by mail-we0-f176.google.com with SMTP id q59so3774020wes.35 for ; Sat, 03 May 2014 19:48:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=4RfkdfFIafN5RV4BO34VxL6JEWLwws6/GpdvvQJsppA=; b=l5wJPvCeYr/yksXnyLgu4H1RDRemUeaOC/jxDIL536RaTYZQ63NVFEhhMv7h+bnqsL A2QF9XIv9bJHs7PqMHs1KLkBCEw0wRizL6ldkv/hxkL42P0/M+vy2shrFgvS7SzihHTL mm+zk2uO/DArB/ewmU3iDU/LSkspLKSNkGoGV4/z1a3D5m0IwN3Rwx4Fdc9gfmuuUbRy KZQMzwazaQyu3iIFcrGGYrRW4hvgQnSNWXs28NZDXWO1v5WbHgY0OR2EhMP47X9edYT5 d2se9dBcmTyGymMedGgbvHz41UTnmXhc8tkeQR2Mk4BgcZdKwglmRJqLnJ6UtRkGo8QV QY8g== MIME-Version: 1.0 X-Received: by 10.194.6.106 with SMTP id z10mr20842174wjz.1.1399171724961; Sat, 03 May 2014 19:48:44 -0700 (PDT) Received: by 10.216.168.194 with HTTP; Sat, 3 May 2014 19:48:44 -0700 (PDT) In-Reply-To: <20140502180115.I1337@besplex.bde.org> References: <201405011409.s41E9mun075016@svn.freebsd.org> <20140502180115.I1337@besplex.bde.org> Date: Sat, 3 May 2014 23:48:44 -0300 Message-ID: Subject: Re: svn commit: r265191 - head/sys/dev/gpio From: Luiz Otavio O Souza To: Bruce Evans Content-Type: text/plain; charset=UTF-8 Cc: Luiz Otavio O Souza , svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 04 May 2014 02:48:48 -0000 On Fri, May 2, 2014 at 5:34 AM, Bruce Evans wrote: > On Thu, 1 May 2014, Luiz Otavio O Souza wrote: > >> Log: >> Remove unnecessary headers. Sort out the headers. Add a missing header >> on >> ofw_gpiobus.c (it was working because of sys/libkern.h). > > > Do you use /usr/src/tools/tools/kerninclude/kerninclude.sh to find the > unused includes? There are many false positives and negatives which are > hard to find without a lot of testing. kerninclude.sh automates some of > the testing. I think it is rarely used and has rotted. A full universe > build is probably required, but kerninclude.sh is i386-centric and only > tests LINT, GENERIC and GENERIC98 (GENERIC98 last existed in the source > tree in FreeBSD-3, but the script creates it by copying GENERIC and > building with pc98 options). > > I prefer my unusedinc script. It is easier to run 1 on file at a time, > but does less hacking to reduce the false positives and negatives. I wasn't aware of the existence of kerninclude.sh. I usually go with clearly unused headers (not as aggressive as with the automated tools). Much of the code i touch has a lot of copy and paste leftovers. A few examples can be found in sys/etherswitch/arswitch/* which includes iic headers when it isn't needed, but i avoid to make these changes until i have time to test it. Your script seems to be less complex to use and, indeed, has helped to find all the unused includes in these files. As for test, i usually run my changes through a universe build anyway. Most of the last minute changes (which won't pass by universe test) tends to bring issues. > > >> Modified: head/sys/dev/gpio/gpiobus.c >> >> ============================================================================== >> --- head/sys/dev/gpio/gpiobus.c Thu May 1 14:08:19 2014 (r265190) >> +++ head/sys/dev/gpio/gpiobus.c Thu May 1 14:09:47 2014 (r265191) >> @@ -28,21 +28,16 @@ >> __FBSDID("$FreeBSD$"); >> >> #include >> -#include > > > This unsorts the includes. must be included after > , and should be always included, since other headers may > depend on it (for things like KASSERT() in inline functions). Some > broken headers include it nested. This makes it difficult to tell > whether it is used. Some other headers that don't do this may > compile accidentally because they are included after the polluted > ones. Fixed. > >> +#include >> +#include >> +#include >> #include >> #include >> -#include >> -#include > > > is probably used. It is included nested in many headers, > and this is not considered pollution, unlike for , but it > makes it very hard to determine if is included as needed > in other headers and .c files. > > kerninclude.sh attempts to determine if headers like are > needed by doing things like replacing it by an empty header in some > contexts. I suspect this doesn't handle the full complications. > Ideally, the include of in a .c file should be explicit > iff the .c files uses any queue macro. No queue macros are in use in these files. > >> -#include >> +#include >> #include > > > is certainly not needed. It is standard pollution in > , and it is a style bug to not depend on that. It is > also usually a style bug (in kernel code) to include > and not include . Many things depend on the standard > pollution, or might be changed to depend on it. Fixed, this is even documented on style(9)... My bad... > >> Modified: head/sys/dev/gpio/ofw_gpiobus.c >> >> ============================================================================== >> --- head/sys/dev/gpio/ofw_gpiobus.c Thu May 1 14:08:19 2014 >> (r265190) >> +++ head/sys/dev/gpio/ofw_gpiobus.c Thu May 1 14:09:47 2014 >> (r265191) >> @@ -33,17 +33,13 @@ __FBSDID("$FreeBSD$"); >> #include >> #include >> #include >> -#include > > > Correct. is standard pollution in , and it > is a style bug to include it directly. > > >> -#include >> #include >> -#include >> +#include > > > Not using mutexes may indicate missing locking. I think this works because > almost everything in new-bus is Giant-locked and Giant locking hides its > own details very well. Mutexes are in use here, but sys/lock.h and sys/mutex.h comes from gpiobusvar.h (which i may need to validate again). [...] Thanks for the review, it is greatly appreciated. Regards, Luiz