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

From: Eric Blake <eblake@redhat.com>
To: Peter Maydell <peter.maydell@linaro.org>, qemu-devel@nongnu.org
Cc: patches@linaro.org
Subject: Re: [Qemu-devel] [PATCH 4/8] scripts/clean-includes: Enhance to handle header files
Date: Thu, 18 Feb 2016 12:04:51 -0700	[thread overview]
Message-ID: <56C615D3.7080303@redhat.com> (raw)
In-Reply-To: <1455818725-7647-5-git-send-email-peter.maydell@linaro.org>

[-- Attachment #1: Type: text/plain, Size: 3421 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]