[v2,01/19] Use #include "..." for our own headers, <...> for others

System headers should be included with <...>, our own headers with
"...".  Offenders tracked down with an ugly, brittle and probably
buggy Perl script.  Previous iteration was commit a9c94277f0.

Put the cleaned up system header includes first, except for the ones
the next commit will delete.

While there, separate #include from file comment with a blank line,

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 contrib/vhost-user-scsi/vhost-user-scsi.c             | 4 ++--
 hw/ide/sii3112.c                                      | 4 ++--
 hw/s390x/s390-ccw.c                                   | 3 ++-
 include/hw/loader-fit.h                               | 2 +-
 include/hw/registerfields.h                           | 2 +-
 include/hw/s390x/storage-attributes.h                 | 2 +-
 target/i386/hax-all.c                                 | 2 +-
 target/i386/hvf/hvf.c                                 | 2 +-
 target/i386/hvf/x86_decode.c                          | 2 +-
 target/i386/hvf/x86_mmu.c                             | 7 +++----
 target/i386/hvf/x86_task.c                            | 2 +-
 target/s390x/gen-features.c                           | 4 ++--
 target/xtensa/core-dc232b/xtensa-modules.c            | 2 +-
 target/xtensa/core-dc233c/xtensa-modules.c            | 2 +-
 target/xtensa/core-de212/xtensa-modules.c             | 2 +-
 target/xtensa/core-fsf/xtensa-modules.c               | 2 +-
 target/xtensa/core-sample_controller/xtensa-modules.c | 2 +-
 target/xtensa/xtensa-isa.h                            | 2 +-
 vl.c                                                  | 2 +-
 19 files changed, 25 insertions(+), 25 deletions(-)

On 01/31/2018 08:48 AM, Markus Armbruster wrote:
> System headers should be included with <...>, our own headers with
> "...".  Offenders tracked down with an ugly, brittle and probably
> buggy Perl script.  Previous iteration was commit a9c94277f0.
> 
> Put the cleaned up system header includes first, except for the ones
> the next commit will delete.
> 
> While there, separate #include from file comment with a blank line,
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

> +++ b/target/i386/hvf/x86_mmu.c
> @@ -15,18 +15,17 @@
>   * You should have received a copy of the GNU Lesser General Public
>   * License along with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
> +
>  #include "qemu/osdep.h"
> +#include <memory.h>

<memory.h> is an obsolete spelling for the now-universal <string.h>.  We
should NEVER need to include it; scripts/clean-includes should be taught
to blacklist this one.

But that's a matter for another patch; this one does exactly what it
promises, and was mechanical (even if the brittle perl script isn't
published), so it's best to keep it as-is.

Reviewed-by: Eric Blake <eblake@redhat.com>

On 31.01.2018 15:48, Markus Armbruster wrote:
> System headers should be included with <...>, our own headers with
> "...". Offenders tracked down with an ugly, brittle and probably
> buggy Perl script.  Previous iteration was commit a9c94277f0.
> 
> Put the cleaned up system header includes first, except for the ones
> the next commit will delete.

That's a little bit of code churn ... why not delete them here
immediately, or simply ignore these headers here and just delete them in
the next patch?

> While there, separate #include from file comment with a blank line,
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
[...]
> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
> index 0570f597ec..9968974fbe 100644
> --- a/target/s390x/gen-features.c
> +++ b/target/s390x/gen-features.c
> @@ -13,8 +13,8 @@
>   */
>  
>  

While you're at it, remove one of the two blank lines?

> -#include "inttypes.h"
> -#include "stdio.h"
> +#include <inttypes.h>
> +#include <stdio.h>
>  #include "cpu_features_def.h"

Above remarks are just nits, anyway patch looks fine, so:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Thomas Huth <thuth@redhat.com> writes:

> On 31.01.2018 15:48, Markus Armbruster wrote:
>> System headers should be included with <...>, our own headers with
>> "...". Offenders tracked down with an ugly, brittle and probably
>> buggy Perl script.  Previous iteration was commit a9c94277f0.
>> 
>> Put the cleaned up system header includes first, except for the ones
>> the next commit will delete.
>
> That's a little bit of code churn ... why not delete them here
> immediately, or simply ignore these headers here and just delete them in
> the next patch?

Ignore won't do, as scripts/clean-includes won't find them then.

Delete is possible, but requires still more explanation in the commit
message.  Worthwhile?

>> While there, separate #include from file comment with a blank line,
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
> [...]
>> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
>> index 0570f597ec..9968974fbe 100644
>> --- a/target/s390x/gen-features.c
>> +++ b/target/s390x/gen-features.c
>> @@ -13,8 +13,8 @@
>>   */
>>  
>>  
>
> While you're at it, remove one of the two blank lines?

Yes.

>> -#include "inttypes.h"
>> -#include "stdio.h"
>> +#include <inttypes.h>
>> +#include <stdio.h>
>>  #include "cpu_features_def.h"
>
> Above remarks are just nits, anyway patch looks fine, so:
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Thanks!

Eric Blake <eblake@redhat.com> writes:

> On 01/31/2018 08:48 AM, Markus Armbruster wrote:
>> System headers should be included with <...>, our own headers with
>> "...".  Offenders tracked down with an ugly, brittle and probably
>> buggy Perl script.  Previous iteration was commit a9c94277f0.
>> 
>> Put the cleaned up system header includes first, except for the ones
>> the next commit will delete.
>> 
>> While there, separate #include from file comment with a blank line,
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
>> +++ b/target/i386/hvf/x86_mmu.c
>> @@ -15,18 +15,17 @@
>>   * You should have received a copy of the GNU Lesser General Public
>>   * License along with this program; if not, see <http://www.gnu.org/licenses/>.
>>   */
>> +
>>  #include "qemu/osdep.h"
>> +#include <memory.h>
>
> <memory.h> is an obsolete spelling for the now-universal <string.h>.  We
> should NEVER need to include it; scripts/clean-includes should be taught
> to blacklist this one.

"Programming today is a race between software engineers striving to
build bigger and better idiot-proof programs, and the Universe trying to
produce bigger and better idiots.  So far, the Universe is winning."

My point is: if we extended our tooling every time we see a mistake,
we'll drown in tooling.  I think we should limit ourselves to *common*
mistakes.  Is this one common?

> But that's a matter for another patch; this one does exactly what it
> promises, and was mechanical (even if the brittle perl script isn't
> published), so it's best to keep it as-is.

I append my Perl script.

Its simpleminded approach to locating headers in the source tree fails
frequently.  It should really use the compiler to find them.

I reviewed inclusions of headers the script flags as "(included
inconsistently)".  I also reviewed "(included with <>)" where the script
can find a header in the tree.  I patched only obvious mistakes.  There
are a few unobvious cases: headers in tree with the same name as a
system header, e.g. elf.h, io.h.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!


#!/usr/bin/perl -w

use strict;

my %ihdr = ();
my %idir = ();
my @sall = ();
my @sinc = ();

open(my $fh, "-|", "git grep '^[ \t]*#[ \t]*include[ \t]'")
    or die "can't run git grep: $!";
while (<$fh>) {
    m,^(([^:]*)/)?[^:/]*:[ \t]*\#[ \t]*include[ \t]*(["<])([^">]*), or next;
    my $dir = $2 // ".";
    my $delim = $3;
    my $h = $4;
    $ihdr{$h} |= 1 << ($delim eq "<");
    if (exists $idir{$h}) {
	my $aref = $idir{$h};
	push @$aref, $dir unless grep($_ eq $dir, @$aref);
    } else {
	$idir{$h} = [$dir];
    }
}
close ($fh);

open($fh, "-|", "git ls-tree -r --name-only HEAD")
    or die "can't run git ls-tree: $!";
while (<$fh>) {
    chomp;
    push @sall, $_;
}
close ($fh);

@sinc = grep(/^include\//, @sall);

sub pr {
    my ($h, $fn, $src) = @_;

    print "$h -> $fn";
    if ($ihdr{$h} == 3) {
	print " (included inconsistently)";
    } elsif ($src) {
	print " (included with <>)" if ($ihdr{$h} != 1);
    } else {
	print " (included with \"\")" if ($ihdr{$h} != 2);
    }
    print "\n";
}

for my $h (keys %ihdr) {
    $h =~ m,^(\.\./)*(include/)?(.*), or die;
    my $hh = $3;
    my @fn = grep(/^include\/\Q$hh\E$/, @sinc);
    if (@fn) {
	pr($h, $fn[0], 1);
	next;
    }
    @fn = grep(/^\Q$hh\E$/, @sall);
    if (@fn) {
	pr($h, $fn[0], 1);
	next;
    }
    for my $dir (@{$idir{$h}}) {
	next if $dir eq ".";
	print "## $dir/$hh\n";
	@fn = grep(/^\Q$dir\/$hh\E$/, @sall);
	if (@fn) {
	    pr($h, $fn[0], 1);
	} else {
	    pr($h, "? (in $dir)", 0);
	}
    }
}

On 01.02.2018 07:57, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 31.01.2018 15:48, Markus Armbruster wrote:
>>> System headers should be included with <...>, our own headers with
>>> "...". Offenders tracked down with an ugly, brittle and probably
>>> buggy Perl script.  Previous iteration was commit a9c94277f0.
>>>
>>> Put the cleaned up system header includes first, except for the ones
>>> the next commit will delete.
>>
>> That's a little bit of code churn ... why not delete them here
>> immediately, or simply ignore these headers here and just delete them in
>> the next patch?
> 
> Ignore won't do, as scripts/clean-includes won't find them then.
> 
> Delete is possible, but requires still more explanation in the commit
> message.  Worthwhile?

I'd prefer to do it. Otherwise, if you look at the commit log of such a
file later, it just looks weird (first change the quotes, then
immediately delete it afterwards again). But if it's too cumbersome to
rework your patch this way right now, I'm also fine if you keep it as it
currently is.

 Thomas

On 02/01/2018 01:12 AM, Markus Armbruster wrote:

>>> +
>>>  #include "qemu/osdep.h"
>>> +#include <memory.h>
>>
>> <memory.h> is an obsolete spelling for the now-universal <string.h>.  We
>> should NEVER need to include it; scripts/clean-includes should be taught
>> to blacklist this one.
> 
> "Programming today is a race between software engineers striving to
> build bigger and better idiot-proof programs, and the Universe trying to
> produce bigger and better idiots.  So far, the Universe is winning."
> 
> My point is: if we extended our tooling every time we see a mistake,
> we'll drown in tooling.  I think we should limit ourselves to *common*
> mistakes.  Is this one common?

Perhaps not; a tendency when writing a new file is to copy-and-paste a
list of includes from another file, then add more if things still don't
compile.  Since adding memory.h never changes whether compilation will
work (unless you forgot osdep.h in the first place, and thereby missed
string.h), and if we eradicate all current uses of memory.h, new uses
won't have the bad example to copy from.