Re: [Qemu-devel] [PATCH 4/8] scripts/clean-includes: Enhance to handle header files

[Peter Maydell]

On 18 February 2016 at 19:04, Eric Blake <address@hidden> 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 <address@hidden>
>> Reviewed-by: Eric Blake <address@hidden>
>> ---
>>  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