On 18 February 2016 at 19:04, Eric Blake <ebl...@redhat.com> wrote: > On 02/18/2016 11:05 AM, Peter Maydell wrote: >> Enhance clean-includes to handle header files as well as .c source >> files. For headers we merely remove all the redundant #include >> lines, including any includes of qemu/osdep.h itself. >> >> There is a simple mollyguard on the include file processing to >> skip a few key headers like osdep.h itself, to avoid producing >> bad patches if the script is run on every file in include/. >> >> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org> >> Reviewed-by: Eric Blake <ebl...@redhat.com> >> --- >> scripts/clean-includes | 50 >> ++++++++++++++++++++++++++++++++++++++++++-------- >> 1 file changed, 42 insertions(+), 8 deletions(-) > > I already saw your followup explaining about the spurious R-b, so let's > make it real this time :) > >> >> diff --git a/scripts/clean-includes b/scripts/clean-includes >> index 1af1f82..737a5ce 100755 >> --- a/scripts/clean-includes >> +++ b/scripts/clean-includes >> @@ -1,7 +1,8 @@ >> #!/bin/sh -e >> # >> # Clean up QEMU #include lines by ensuring that qemu/osdep.h >> -# is the first include listed. >> +# is the first include listed, and no headers provided by >> +# osdep.h itself are redundantly included. > > Do you want to mention 'is the first include listed in .c files', now > that this cleanup also scrubs .h files? Or, since you go into details > below and this is just the summary, maybe 'is the first include > encountered'?
OK. >> # >> # Copyright (c) 2015 Linaro Limited > > Want to add 2016? Most of the Copyright lines in QEMU source files are probably out of date; I'm not convinced that touching the Copyright line for the first patch to each source file each year really gains us anything. >> # >> @@ -22,6 +23,11 @@ >> >> # This script requires Coccinelle to be installed. >> >> +# .c files will have the osdep.h included added, and redundant >> +# includes removed. >> +# .h files will have redundant includes (including includes of osdep.h) >> +# removed. > > Maybe a note here that "this is because any other .h file will not be > included by a .c file until after osdep.h" ? Or is that too verbose? Feels a touch over-verbose to me. >> + case "$f" in >> + *.c) >> + MODE=c >> + ;; >> + >> *include/qemu/osdep.h|include/qemu/compiler.h|include/config.h|include/standard-headers/) > > Spaces around | may make this more legible, and doesn't affect correctness. OK. >> + >> + if [ "$MODE" = "c" ]; then >> + # First, use coccinelle to add qemu/osdep.h before the first existing >> include > > Should the tool name be capitalized as Coccinelle? OK. >> + # (this will add two lines if the file uses both "..." and <...> >> #includes, >> + # but we will remove the extras in the next step) >> + spatch --in-place --no-show-diff --cocci-file "$COCCIFILE" "$f" >> + >> + # Now remove any duplicate osdep.h includes >> + perl -n -i -e 'print if !/#include "qemu\/osdep.h"/ || !$n++;' "$f" > > Why two spaces before -e? Accidental. > I can understand the use of perl here (detecting duplicates lines is > doable, but a lot harder, in sed),... > >> + else >> + # Remove includes of osdep.h itself >> + perl -n -i -e 'print if !/\s*#\s*include\s*(["<][^>"]*[">])/ || >> + ! (grep { $_ eq $1 } qw ("qemu/osdep.h"))' "$f" > > ...but this could be shortened, if you wanted: > > sed -i '/#[[:space:]]*include[[:space:]]*["<]qemu/osdep.h[">]/d' "$f" The next perl line (only partially visible in the diff context) is doing basically the same job for a larger set of header files, so I preferred to retain the same code to do the same thing, even if in this case there's only one header in the list. > My findings are minor; functionally, your patch is sane, so the > accidental R-b can now be treated as real, if you don't want to respin. Thanks. I'll fix the minor space/comment issues above, but won't resend unless I need to redo the series for some other reason. -- PMM