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'?
>
>>  #
>>  # 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