| Message ID | 20231218-libstringheader-v3-2-500bd58f0f75@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | shrink lib/string.i via IWYU | expand |
On Mon, Dec 18, 2023 at 06:44:48PM +0000, tanzirh@google.com wrote: > This diff uses an open source tool include-what-you-use (IWYU) to modify > the include list changing indirect includes to direct includes. > IWYU is implemented using the IWYUScripts github repository which is a tool that is > currently undergoing development. These changes seek to improve build times. > > This change to lib/string.c resulted in a preprocessed size of > lib/string.i from 26371 lines to 5263 lines (-80%) for the x86 > defconfig. ... > #define __NO_FORTIFY > #include <linux/types.h> > -#include <linux/string.h> > -#include <linux/ctype.h> > -#include <linux/kernel.h> > -#include <linux/export.h> > +#include <linux/bits.h> > #include <linux/bug.h> > #include <linux/errno.h> > -#include <linux/slab.h> > - Why this blank is removed? > +#include <linux/linkage.h> > +#include <linux/stddef.h> > +#include <linux/string.h> > +#include <linux/ctype.h> > #include <asm/unaligned.h> > -#include <asm/byteorder.h> > +#include <asm/rwonce.h> > #include <asm/word-at-a-time.h> > #include <asm/page.h> Sort this group alphabetically as well. > +#include <vdso/limits.h> Just use linux/limits.h. VDSO is a very special UAPI case. So it's even stricter rule than for asm/ for using anything from there. Expected result: #include <linux/bits.h> #include <linux/bug.h> #include <linux/ctype.h> #include <linux/errno.h> #include <linux/limits.h> #include <linux/linkage.h> #include <linux/stddef.h> #include <linux/string.h> #include <linux/types.h> #include <asm/page.h> #include <asm/rwonce.h> #include <asm/unaligned.h> #include <asm/word-at-a-time.h>
On Tue, Dec 19, 2023 at 7:55 AM Andy Shevchenko <andy@kernel.org> wrote: > > On Mon, Dec 18, 2023 at 06:44:48PM +0000, tanzirh@google.com wrote: > > This diff uses an open source tool include-what-you-use (IWYU) to modify > > the include list changing indirect includes to direct includes. > > IWYU is implemented using the IWYUScripts github repository which is a tool that is > > currently undergoing development. These changes seek to improve build times. > > > > This change to lib/string.c resulted in a preprocessed size of > > lib/string.i from 26371 lines to 5263 lines (-80%) for the x86 > > defconfig. > > ... > > > #define __NO_FORTIFY > > #include <linux/types.h> > > -#include <linux/string.h> > > -#include <linux/ctype.h> > > -#include <linux/kernel.h> > > -#include <linux/export.h> > > +#include <linux/bits.h> > > #include <linux/bug.h> > > #include <linux/errno.h> > > -#include <linux/slab.h> > > > - > > Why this blank is removed? The automation isn't aware of any convention around having blank lines separate linux/* vs asm/*. Is that a convention we have throughout the kernel, or just this file? If we rerun the automation on this file after Tanzir's patch here, we get no further changes. If we manually touch up the results, then rerun the automation, it will undo the manual touch ups. I don't mind saying "you might have to manually touch up the results of the automation to comply with <link to documentation on stated kernel policy/style guide>" but on the other hand I also think it would be nice if other folks run the automation so that they don't get additional changes. I'd like to avoid drive-by patches that just undo any manual touch ups. > > > +#include <linux/linkage.h> > > +#include <linux/stddef.h> > > +#include <linux/string.h> > > +#include <linux/ctype.h> > > #include <asm/unaligned.h> > > -#include <asm/byteorder.h> > > +#include <asm/rwonce.h> > > #include <asm/word-at-a-time.h> > > #include <asm/page.h> > > Sort this group alphabetically as well. By default the automation sorts the result. I asked Tanzir to explicitly disable that; otherwise the resulting diffstat is hard to tell precisely what was removed/added vs simply moved. If he kept the default behavior, I highly suspect the feedback would have been along the lines of "please don't sort the result; I can't tell what moved vs was added or removed." Perhaps we should add another commit on the series that manually sorts the results _after_ the automation? Do we have anything in Documentation/process/coding-style.rst about sorting headers? There's a blip about clang-format sorting them, but we don't have strong guidance along the lines of "you ought to sort your includes (when you don't have special cases like x-macros)." > > > +#include <vdso/limits.h> > > Just use linux/limits.h. > > VDSO is a very special UAPI case. So it's even stricter rule > than for asm/ for using anything from there. We can add a special rule for this, Tanzir, please add. And remember to re-measure the results of that change for this patch's commit message. > > Expected result: > > #include <linux/bits.h> > #include <linux/bug.h> > #include <linux/ctype.h> > #include <linux/errno.h> > #include <linux/limits.h> > #include <linux/linkage.h> > #include <linux/stddef.h> > #include <linux/string.h> > #include <linux/types.h> > > #include <asm/page.h> > #include <asm/rwonce.h> > #include <asm/unaligned.h> > #include <asm/word-at-a-time.h> > > -- > With Best Regards, > Andy Shevchenko > >
On Tue, Dec 19, 2023 at 08:43:01AM -0800, Nick Desaulniers wrote: > On Tue, Dec 19, 2023 at 7:55 AM Andy Shevchenko <andy@kernel.org> wrote: > > On Mon, Dec 18, 2023 at 06:44:48PM +0000, tanzirh@google.com wrote: ... > > > #define __NO_FORTIFY > > > #include <linux/types.h> > > > -#include <linux/string.h> > > > -#include <linux/ctype.h> > > > -#include <linux/kernel.h> > > > -#include <linux/export.h> > > > +#include <linux/bits.h> > > > #include <linux/bug.h> > > > #include <linux/errno.h> > > > -#include <linux/slab.h> > > > > > - > > > > Why this blank is removed? > > The automation isn't aware of any convention around having blank lines > separate linux/* vs asm/*. Is that a convention we have throughout > the kernel, or just this file? There is no documented conventions like this. Probably one has to add some, let's say, recommendations. This way (alphabetical ordering and grouping) it is easier to maintain and in long term makes easier (less possible conflicts) when backporting. > If we rerun the automation on this file after Tanzir's patch here, we > get no further changes. If we manually touch up the results, then > rerun the automation, it will undo the manual touch ups. > > I don't mind saying "you might have to manually touch up the results > of the automation to comply with <link to documentation on stated > kernel policy/style guide>" but on the other hand I also think it > would be nice if other folks run the automation so that they don't get > additional changes. I'd like to avoid drive-by patches that just undo > any manual touch ups. I agree with this, but we have many maintainers and reviewers and some ask one way, the others another way and so on... Without documentation one can use a common sense and rationale behind it. I don't really care about blank lines (however grouping is still done as of today in this file), but I do care about ordering and grouping. ... > > > +#include <linux/linkage.h> > > > +#include <linux/stddef.h> > > > +#include <linux/string.h> > > > +#include <linux/ctype.h> > > > #include <asm/unaligned.h> > > > -#include <asm/byteorder.h> > > > +#include <asm/rwonce.h> > > > #include <asm/word-at-a-time.h> > > > #include <asm/page.h> > > > > Sort this group alphabetically as well. > > By default the automation sorts the result. I asked Tanzir to > explicitly disable that; otherwise the resulting diffstat is hard to > tell precisely what was removed/added vs simply moved. > > If he kept the default behavior, I highly suspect the feedback would > have been along the lines of "please don't sort the result; I can't > tell what moved vs was added or removed." Do you have such examples IRL? In any case, how would you satisfy the maintainer's request which asks for sorting? > Perhaps we should add another commit on the series that manually sorts > the results _after_ the automation? I have no objections for this way (doing it in a two separate changes). With the proposed you can satisfy any maintainer basically. Ones who do not want sorting, may skip that patch, indeed. > Do we have anything in Documentation/process/coding-style.rst about > sorting headers? There's a blip about clang-format sorting them, but > we don't have strong guidance along the lines of "you ought to sort > your includes (when you don't have special cases like x-macros)." Actually there is a rising trend to follow clang-format. I have no idea what it does, but my rationale is explained above. ... > > > +#include <vdso/limits.h> > > > > Just use linux/limits.h. > > > > VDSO is a very special UAPI case. So it's even stricter rule > > than for asm/ for using anything from there. > > We can add a special rule for this, Tanzir, please add. And remember > to re-measure the results of that change for this patch's commit > message. Yes, please do. ... > > Expected result: > > > > #include <linux/bits.h> > > #include <linux/bug.h> > > #include <linux/ctype.h> > > #include <linux/errno.h> > > #include <linux/limits.h> > > #include <linux/linkage.h> > > #include <linux/stddef.h> > > #include <linux/string.h> > > #include <linux/types.h> > > > > #include <asm/page.h> > > #include <asm/rwonce.h> > > #include <asm/unaligned.h> > > #include <asm/word-at-a-time.h> So, let's have above with one or two patches.
diff --git a/lib/string.c b/lib/string.c index be26623953d2..6e24a4967dd7 100644 --- a/lib/string.c +++ b/lib/string.c @@ -16,18 +16,18 @@ #define __NO_FORTIFY #include <linux/types.h> -#include <linux/string.h> -#include <linux/ctype.h> -#include <linux/kernel.h> -#include <linux/export.h> +#include <linux/bits.h> #include <linux/bug.h> #include <linux/errno.h> -#include <linux/slab.h> - +#include <linux/linkage.h> +#include <linux/stddef.h> +#include <linux/string.h> +#include <linux/ctype.h> #include <asm/unaligned.h> -#include <asm/byteorder.h> +#include <asm/rwonce.h> #include <asm/word-at-a-time.h> #include <asm/page.h> +#include <vdso/limits.h> #ifndef __HAVE_ARCH_STRNCASECMP /**