| Message ID | 1455818725-7647-5-git-send-email-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18 February 2016 at 18:05, Peter Maydell <peter.maydell@linaro.org> 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.maydell@linaro.org> > Reviewed-by: Eric Blake <eblake@redhat.com> Hmm, I'm not sure why this ended up with an R-b tag from Eric because I'm pretty sure this is the first time I've sent it to the list. Sorry about that, and please ignore that r-b tag... thanks -- PMM
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.maydell@linaro.org> > Reviewed-by: Eric Blake <eblake@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'? > # > # Copyright (c) 2015 Linaro Limited Want to add 2016? > # > @@ -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? > + 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. > + > + 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? > + # (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? 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" 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.
On 18 February 2016 at 19:04, Eric Blake <eblake@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.maydell@linaro.org> >> Reviewed-by: Eric Blake <eblake@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
On 18 February 2016 at 19:04, Eric Blake <eblake@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.maydell@linaro.org> >> Reviewed-by: Eric Blake <eblake@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'? > >> # >> # Copyright (c) 2015 Linaro Limited > > Want to add 2016? > >> # >> @@ -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? > >> + 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. Your comment here reveals a bug, incidentally -- I meant (with the '*' at the front) to make this work whether $f was "include/foo.h" or "./include/foo.h" or "/path/to/qemu/include/foo.h". I guess I want *include/qemu/osdep.h | \ *include/qemu/compiler.h | \ *include/config.h | \ *include/standard-headers/ ) (including some line breaks to help readability a little). thanks -- PMM
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. # # Copyright (c) 2015 Linaro Limited # @@ -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. +# Other files (including C++ and ObjectiveC) can't be handled by this script. # The following one-liner may be handy for finding files to run this on. # However some caution is required regarding files that might be part @@ -73,13 +79,41 @@ EOT for f in "$@"; do - # First, use coccinelle to add qemu/osdep.h before the first existing include - # (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" + case "$f" in + *.c) + MODE=c + ;; + *include/qemu/osdep.h|include/qemu/compiler.h|include/config.h|include/standard-headers/) + # Removing include lines from osdep.h itself would be counterproductive. + echo "SKIPPING $f (special case header)" + continue + ;; + *include/standard-headers/*) + echo "SKIPPING $f (autogenerated header)" + continue + ;; + *.h) + MODE=h + ;; + *) + echo "WARNING: ignoring $f (cannot handle non-C files)" + continue + ;; + esac + + if [ "$MODE" = "c" ]; then + # First, use coccinelle to add qemu/osdep.h before the first existing include + # (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" + 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" + fi # Remove includes that osdep.h already provides perl -n -i -e 'print if !/\s*#\s*include\s*(["<][^>"]*[">])/ ||