Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 02:54:37PM +0100, Max Reitz wrote:
> But I guess the main advantage with using this rule I see is that it's
> better for people reading the code.  It's just nice to know whether a
> file belongs to qemu or not by just looking at the #include statement.
> (Note that this implies that it is indeed more difficult to determine
> whether a header belongs to qemu than whether it sits in the same
> directory as the C file, though!)  So I think the old (current) rule is
> better for reading code, Michael's rule would be better for writing
> code.  I think reading code is what should be easier.

How about prefixing all internal headers with qemu/ ?

That's a way to avoid namespace collisions that is actually standard.

-- 
MST 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 11:12:00AM -0500, Eric Blake wrote:
> 
> Why can't we fix Makefile to include BOTH the build and the source
> directories (to pick up generated files first, and then version-controlled
> files), and possibly include logic to simplify to a single -I instead of two
> when doing in-tree builds?

That's the issue.  Whatever you do, #include "" pulls in the current
directory first.

-- 
MST

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 11:12:00AM -0500, Eric Blake wrote:
> On 03/19/2018 08:54 PM, Michael S. Tsirkin wrote:
> > QEMU coding style at the moment asks for all non-system
> > include files to be used with #include "foo.h".
> 
> [I'm replying without having read the rest of the thread, so bear with me if
> I repeat some of the other comments that have already been made]
> 
> And Markus even just did a cleanup along those lines.
> 
> > However this rule actually does not make sense and
> > creates issues for when the included file is generated.
> > 
> > In C, include "file" means look in current directory,
> > then on include search path. Current directory here
> > means the source file directory.
> > By comparison include  means look on include search path.
> 
> It's also nice when "file" means file belonging to our project, and 
> means 3rd-party file.  So we have to choose which semantics are easier;
> perhaps better Makefile rules that prevent us from seeing stale files is a
> better solution than figuring out which files are generated.

That's what I've attempted here:

  https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg05421.html


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Eric Blake]

On 03/19/2018 08:54 PM, Michael S. Tsirkin wrote:

QEMU coding style at the moment asks for all non-system
include files to be used with #include "foo.h".


[I'm replying without having read the rest of the thread, so bear with 
me if I repeat some of the other comments that have already been made]


And Markus even just did a cleanup along those lines.


However this rule actually does not make sense and
creates issues for when the included file is generated.

In C, include "file" means look in current directory,
then on include search path. Current directory here
means the source file directory.
By comparison include  means look on include search path.


It's also nice when "file" means file belonging to our project, and 
 means 3rd-party file.  So we have to choose which semantics are 
easier; perhaps better Makefile rules that prevent us from seeing stale 
files is a better solution than figuring out which files are generated.




As generated files are not in the search directory (unless the build
directory happens to match the source directory),


Why can't we fix Makefile to include BOTH the build and the source 
directories (to pick up generated files first, and then 
version-controlled files), and possibly include logic to simplify to a 
single -I instead of two when doing in-tree builds?



it does not make sense
to include them with "" - doing so is merely more work for preprocessor
and a source or errors if a stale file happens to exist in the source
directory.

This changes include directives for all generated files, across the
tree. The idea is to avoid sending a huge amount of email.  But when
merging, the changes will be split with one commit per file, e.g. for
ease of bisect in case of build failures, and to ease merging.

Note that should some generated files be missed by this tree-wide
refactoring, it isn't a big deal - this merely maintains the status quo,
and this can be addressed by a separate patch on top.


I'm not sure I'm a fan of this patch yet, if there may be other 
alternatives that solve the real issue of stale generated files breaking 
incremental builds.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 01:58:32PM +, Daniel P. Berrangé wrote:
> > That's the C language for you though.  For better or worse, these are
> > the rules that K came up with.
> 
> No one seriously tries to keep compliance with original K C these days,
> things have moved on massively since then.

Not where the preprocessor is involved.

> Regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 03:50:02PM +0200, Michael S. Tsirkin wrote:
> On Tue, Mar 20, 2018 at 01:41:17PM +, Daniel P. Berrangé wrote:
> > On Tue, Mar 20, 2018 at 02:32:16PM +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > > So for these, we should use "".  None of these are generated files 
> > > > > though.
> > > > 
> > > > That leads to crazy inconsistent message for developers where 50% of 
> > > > QEMU
> > > > header files must use <> and the other 50% of header files must use "".
> > > 
> > > The rules are pretty simple though:
> > > 
> > >(1) Headers which are generated use <>.
> > >(2) Headers which are in include/ use <>.
> > >(3) Headers sitting in the same directory as the source files use "".
> > 
> > We have 1200 header files in QEMU - a developer might just reasonably 
> > remember
> > which header file is a QEMU header, vs which is a 3rd party system header.
> > Expecting devs to remember which of those 3 buckets each QEMU header falls
> > under is unreasonable IMHO. 
> 
> That's the C language for you though.  For better or worse, these are
> the rules that K came up with.

No one seriously tries to keep compliance with original K C these days,
things have moved on massively since then.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Max Reitz]

On 2018-03-20 14:41, Daniel P. Berrangé wrote:
> On Tue, Mar 20, 2018 at 02:32:16PM +0100, Gerd Hoffmann wrote:
>>   Hi,
>>
 So for these, we should use "".  None of these are generated files though.
>>>
>>> That leads to crazy inconsistent message for developers where 50% of QEMU
>>> header files must use <> and the other 50% of header files must use "".
>>
>> The rules are pretty simple though:
>>
>>(1) Headers which are generated use <>.
>>(2) Headers which are in include/ use <>.
>>(3) Headers sitting in the same directory as the source files use "".
> 
> We have 1200 header files in QEMU - a developer might just reasonably remember
> which header file is a QEMU header, vs which is a 3rd party system header.
> Expecting devs to remember which of those 3 buckets each QEMU header falls
> under is unreasonable IMHO. 

I agree with this in principle, but I don't find it unreasonable for
someone to look up whether a header file exists in the same directory.
I find that much simpler than to look up whether it is a system header,
actually.

And I have to agree with Michael that his rule when to use "" (if the
file is in the same directory) and when to use <> (otherwise) is
actually what every C developer might do by instinct.  (I guess it's
also easier to check in a script than the original guideline?)

However, I also think that if any problem arises because "" was used for
a generated file and that then uses a stale file, the bug is somewhere
else.  And I think that while Michael's proposal is the more intuitive
(C) way, it is a tiny bit more complicated to explain than 'Use "" for
qemu headers, <> for everything else'.

But I guess the main advantage with using this rule I see is that it's
better for people reading the code.  It's just nice to know whether a
file belongs to qemu or not by just looking at the #include statement.
(Note that this implies that it is indeed more difficult to determine
whether a header belongs to qemu than whether it sits in the same
directory as the C file, though!)  So I think the old (current) rule is
better for reading code, Michael's rule would be better for writing
code.  I think reading code is what should be easier.

But since that may be eaten up by build breakages due to stale files, I
don't have a strong opinion either way.  I just wanted to chime in
because in my opinion 'Use "" for headers in the same directory, <> for
everything else' is by no means an unreasonably complicated rule for
people writing code.

Max



signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 02:46:46PM +0100, Thomas Huth wrote:
> On 20.03.2018 14:32, Gerd Hoffmann wrote:
> >   Hi,
> > 
> >>> So for these, we should use "".  None of these are generated files though.
> >>
> >> That leads to crazy inconsistent message for developers where 50% of QEMU
> >> header files must use <> and the other 50% of header files must use "".
> > 
> > The rules are pretty simple though:
> > 
> >(1) Headers which are generated use <>.
> >(2) Headers which are in include/ use <>.
> >(3) Headers sitting in the same directory as the source files use "".
> 
> Ugh, no. Please don't. The normal way of including header files in QEMU

Stress on "in QEMU".

> is to use "" - also for headers that are not in the same directory. For
> example just do a
> 
>  grep -r '^#include.*hw/' hw/
> 
> from the top directory and you'll see what I mean. Changing that rule is
> crazy.
> So please, let's just fix the configure script to detect some
> more stale files in the source tree, and we're done.
> 
>  Thomas

The rule probably served a useful purpose when all
headers where in the same directory as the source.
we moved them out so that is no longer the case.

-- 
MST

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Thomas Huth]

On 20.03.2018 14:32, Gerd Hoffmann wrote:
>   Hi,
> 
>>> So for these, we should use "".  None of these are generated files though.
>>
>> That leads to crazy inconsistent message for developers where 50% of QEMU
>> header files must use <> and the other 50% of header files must use "".
> 
> The rules are pretty simple though:
> 
>(1) Headers which are generated use <>.
>(2) Headers which are in include/ use <>.
>(3) Headers sitting in the same directory as the source files use "".

Ugh, no. Please don't. The normal way of including header files in QEMU
is to use "" - also for headers that are not in the same directory. For
example just do a

 grep -r '^#include.*hw/' hw/

from the top directory and you'll see what I mean. Changing that rule is
crazy. So please, let's just fix the configure script to detect some
more stale files in the source tree, and we're done.

 Thomas

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 02:32:16PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > So for these, we should use "".  None of these are generated files though.
> > 
> > That leads to crazy inconsistent message for developers where 50% of QEMU
> > header files must use <> and the other 50% of header files must use "".
> 
> The rules are pretty simple though:
> 
>(1) Headers which are generated use <>.
>(2) Headers which are in include/ use <>.
>(3) Headers sitting in the same directory as the source files use "".

We have 1200 header files in QEMU - a developer might just reasonably remember
which header file is a QEMU header, vs which is a 3rd party system header.
Expecting devs to remember which of those 3 buckets each QEMU header falls
under is unreasonable IMHO. 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 03:54:36AM +0200, Michael S. Tsirkin wrote:
> QEMU coding style at the moment asks for all non-system
> include files to be used with #include "foo.h".
> However this rule actually does not make sense and
> creates issues for when the included file is generated.
> 
> In C, include "file" means look in current directory,
> then on include search path. Current directory here
> means the source file directory.
> By comparison include  means look on include search path.
> 
> As generated files are not in the search directory (unless the build
> directory happens to match the source directory), it does not make sense
> to include them with "" - doing so is merely more work for preprocessor
> and a source or errors if a stale file happens to exist in the source
> directory.
> 
> This changes include directives for all generated files, across the
> tree. The idea is to avoid sending a huge amount of email.  But when
> merging, the changes will be split with one commit per file, e.g. for
> ease of bisect in case of build failures, and to ease merging.
> 
> Note that should some generated files be missed by this tree-wide
> refactoring, it isn't a big deal - this merely maintains the status quo,
> and this can be addressed by a separate patch on top.
> 
> Signed-off-by: Michael S. Tsirkin 


For the record, the stated advantage is that one can
have a header file that happens to match the system
header.

To put it bluntly that does not work as designed.

For example, if a system header foo.h somewhere has #include 
then the compiler will happily pull in our own version (since that is in
the -I path) and completely ignore the system one, breaking things in
the process.

When does it make sense to use include ""? When the header is
a directory-specific one, located with the source.
This approach would both be enforced by the compiler
and help people know where to find the header.

-- 
MST

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 12:39:00PM +, Daniel P. Berrangé wrote:
> On Tue, Mar 20, 2018 at 02:28:42PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Mar 20, 2018 at 12:18:41PM +, Daniel P. Berrangé wrote:
> > > On Tue, Mar 20, 2018 at 02:12:24PM +0200, Michael S. Tsirkin wrote:
> > > > On Tue, Mar 20, 2018 at 09:44:06AM +, Daniel P. Berrangé wrote:
> > > > > On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote:
> > > > > > Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> > > > > > > QEMU coding style at the moment asks for all non-system
> > > > > > > include files to be used with #include "foo.h".
> > > > > > > However this rule actually does not make sense and
> > > > > > > creates issues for when the included file is generated.
> > > > > > 
> > > > > > If you change that, we can have issue when a system include has the 
> > > > > > same
> > > > > > name as our local include. With "", system header are taken 
> > > > > > first.
> > > > > 
> > > > > > > In C, include "file" means look in current directory,
> > > > > > > then on include search path. Current directory here
> > > > > > > means the source file directory.
> > > > > > > By comparison include  means look on include search path.
> > > > > > 
> > > > > > Not exactly, there is the notion of "system header" too.
> > > > > > 
> > > > > > https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
> > > > > > 
> > > > > > #include 
> > > > > > This variant is used for system header files. It searches for a file
> > > > > > named file in a standard list of system directories. You can prepend
> > > > > > directories to this list with the -I option (see Invocation).
> > > > > > 
> > > > > > #include "file"
> > > > > > This variant is used for header files of your own program. It 
> > > > > > searches
> > > > > > for a file named file first in the directory containing the current
> > > > > > file, then in the quote directories and then the same directories 
> > > > > > used
> > > > > > for . You can prepend directories to the list of quote 
> > > > > > directories
> > > > > > with the -iquote option.
> > > > > > 
> > > > > > > As generated files are not in the search directory (unless the 
> > > > > > > build
> > > > > > > directory happens to match the source directory), it does not 
> > > > > > > make sense
> > > > > > > to include them with "" - doing so is merely more work for 
> > > > > > > preprocessor
> > > > > > > and a source or errors if a stale file happens to exist in the 
> > > > > > > source
> > > > > > > directory.
> > > > > > 
> > > > > > I agree there is a problem with stale files. But linux, for 
> > > > > > instance,
> > > > > > asks for a "make mrproper" to avoid this.
> > > > > 
> > > > > We can follow what autoconf does, and add a check to configure to see 
> > > > > if
> > > > > there are generated files left in the source dir, when configuring 
> > > > > with
> > > > > builddir != srcdir, and exit with error, telling user to clean their
> > > > > src dir first.
> > > > > 
> > > > > > > This changes include directives for all generated files, across 
> > > > > > > the
> > > > > > > tree. The idea is to avoid sending a huge amount of email.  But 
> > > > > > > when
> > > > > > > merging, the changes will be split with one commit per file, e.g. 
> > > > > > > for
> > > > > > > ease of bisect in case of build failures, and to ease merging.
> > > > > > > 
> > > > > > > Note that should some generated files be missed by this tree-wide
> > > > > > > refactoring, it isn't a big deal - this merely maintains the 
> > > > > > > status quo,
> > > > > > > and this can be addressed by a separate patch on top.
> > > > > > > 
> > > > > > > Signed-off-by: Michael S. Tsirkin 
> > > > > > 
> > > > > > I think your idea conflicts with what Markus has started to do:
> > > > > 
> > > > > Yes, I don't think we should revert what Markus started.   Both ways 
> > > > > of
> > > > > referencing QEMU headers have downsides, but I think "..." has fewer
> > > > > downsides that "<">.
> > > > 
> > > > Could you please explain what the advantage of "" is?
> > > > It seems to be gone since we moved headers away from
> > > > source.
> > > 
> > > We moved *some* headers into the include/ directory tree.
> > > 
> > > I still count 650+ headers which are alongside the .c files.
> > 
> > So for these, we should use "".  None of these are generated files though.
> 
> That leads to crazy inconsistent message for developers where 50% of QEMU
> header files must use <> and the other 50% of header files must use "".
> Having a consistent message for developers is one of the key reasons why
> Markus submitted the patches to standardize on the use of "" for QEMU
> header files, leaving <> for system headers & external dependancies.
> 
> Regards,
> Daniel

I guess it's in the eye of the beholder. The simple rule since days of
K is that "" looks in the current directory of the source.
Whoever learned C knows this.

So use "" if your 

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 02:28:42PM +0200, Michael S. Tsirkin wrote:
> On Tue, Mar 20, 2018 at 12:18:41PM +, Daniel P. Berrangé wrote:
> > On Tue, Mar 20, 2018 at 02:12:24PM +0200, Michael S. Tsirkin wrote:
> > > On Tue, Mar 20, 2018 at 09:44:06AM +, Daniel P. Berrangé wrote:
> > > > On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote:
> > > > > Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> > > > > > QEMU coding style at the moment asks for all non-system
> > > > > > include files to be used with #include "foo.h".
> > > > > > However this rule actually does not make sense and
> > > > > > creates issues for when the included file is generated.
> > > > > 
> > > > > If you change that, we can have issue when a system include has the 
> > > > > same
> > > > > name as our local include. With "", system header are taken 
> > > > > first.
> > > > 
> > > > > > In C, include "file" means look in current directory,
> > > > > > then on include search path. Current directory here
> > > > > > means the source file directory.
> > > > > > By comparison include  means look on include search path.
> > > > > 
> > > > > Not exactly, there is the notion of "system header" too.
> > > > > 
> > > > > https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
> > > > > 
> > > > > #include 
> > > > > This variant is used for system header files. It searches for a file
> > > > > named file in a standard list of system directories. You can prepend
> > > > > directories to this list with the -I option (see Invocation).
> > > > > 
> > > > > #include "file"
> > > > > This variant is used for header files of your own program. It searches
> > > > > for a file named file first in the directory containing the current
> > > > > file, then in the quote directories and then the same directories used
> > > > > for . You can prepend directories to the list of quote 
> > > > > directories
> > > > > with the -iquote option.
> > > > > 
> > > > > > As generated files are not in the search directory (unless the build
> > > > > > directory happens to match the source directory), it does not make 
> > > > > > sense
> > > > > > to include them with "" - doing so is merely more work for 
> > > > > > preprocessor
> > > > > > and a source or errors if a stale file happens to exist in the 
> > > > > > source
> > > > > > directory.
> > > > > 
> > > > > I agree there is a problem with stale files. But linux, for instance,
> > > > > asks for a "make mrproper" to avoid this.
> > > > 
> > > > We can follow what autoconf does, and add a check to configure to see if
> > > > there are generated files left in the source dir, when configuring with
> > > > builddir != srcdir, and exit with error, telling user to clean their
> > > > src dir first.
> > > > 
> > > > > > This changes include directives for all generated files, across the
> > > > > > tree. The idea is to avoid sending a huge amount of email.  But when
> > > > > > merging, the changes will be split with one commit per file, e.g. 
> > > > > > for
> > > > > > ease of bisect in case of build failures, and to ease merging.
> > > > > > 
> > > > > > Note that should some generated files be missed by this tree-wide
> > > > > > refactoring, it isn't a big deal - this merely maintains the status 
> > > > > > quo,
> > > > > > and this can be addressed by a separate patch on top.
> > > > > > 
> > > > > > Signed-off-by: Michael S. Tsirkin 
> > > > > 
> > > > > I think your idea conflicts with what Markus has started to do:
> > > > 
> > > > Yes, I don't think we should revert what Markus started.   Both ways of
> > > > referencing QEMU headers have downsides, but I think "..." has fewer
> > > > downsides that "<">.
> > > 
> > > Could you please explain what the advantage of "" is?
> > > It seems to be gone since we moved headers away from
> > > source.
> > 
> > We moved *some* headers into the include/ directory tree.
> > 
> > I still count 650+ headers which are alongside the .c files.
> 
> So for these, we should use "".  None of these are generated files though.

That leads to crazy inconsistent message for developers where 50% of QEMU
header files must use <> and the other 50% of header files must use "".
Having a consistent message for developers is one of the key reasons why
Markus submitted the patches to standardize on the use of "" for QEMU
header files, leaving <> for system headers & external dependancies.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 12:18:41PM +, Daniel P. Berrangé wrote:
> On Tue, Mar 20, 2018 at 02:12:24PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Mar 20, 2018 at 09:44:06AM +, Daniel P. Berrangé wrote:
> > > On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote:
> > > > Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> > > > > QEMU coding style at the moment asks for all non-system
> > > > > include files to be used with #include "foo.h".
> > > > > However this rule actually does not make sense and
> > > > > creates issues for when the included file is generated.
> > > > 
> > > > If you change that, we can have issue when a system include has the same
> > > > name as our local include. With "", system header are taken first.
> > > 
> > > > > In C, include "file" means look in current directory,
> > > > > then on include search path. Current directory here
> > > > > means the source file directory.
> > > > > By comparison include  means look on include search path.
> > > > 
> > > > Not exactly, there is the notion of "system header" too.
> > > > 
> > > > https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
> > > > 
> > > > #include 
> > > > This variant is used for system header files. It searches for a file
> > > > named file in a standard list of system directories. You can prepend
> > > > directories to this list with the -I option (see Invocation).
> > > > 
> > > > #include "file"
> > > > This variant is used for header files of your own program. It searches
> > > > for a file named file first in the directory containing the current
> > > > file, then in the quote directories and then the same directories used
> > > > for . You can prepend directories to the list of quote directories
> > > > with the -iquote option.
> > > > 
> > > > > As generated files are not in the search directory (unless the build
> > > > > directory happens to match the source directory), it does not make 
> > > > > sense
> > > > > to include them with "" - doing so is merely more work for 
> > > > > preprocessor
> > > > > and a source or errors if a stale file happens to exist in the source
> > > > > directory.
> > > > 
> > > > I agree there is a problem with stale files. But linux, for instance,
> > > > asks for a "make mrproper" to avoid this.
> > > 
> > > We can follow what autoconf does, and add a check to configure to see if
> > > there are generated files left in the source dir, when configuring with
> > > builddir != srcdir, and exit with error, telling user to clean their
> > > src dir first.
> > > 
> > > > > This changes include directives for all generated files, across the
> > > > > tree. The idea is to avoid sending a huge amount of email.  But when
> > > > > merging, the changes will be split with one commit per file, e.g. for
> > > > > ease of bisect in case of build failures, and to ease merging.
> > > > > 
> > > > > Note that should some generated files be missed by this tree-wide
> > > > > refactoring, it isn't a big deal - this merely maintains the status 
> > > > > quo,
> > > > > and this can be addressed by a separate patch on top.
> > > > > 
> > > > > Signed-off-by: Michael S. Tsirkin 
> > > > 
> > > > I think your idea conflicts with what Markus has started to do:
> > > 
> > > Yes, I don't think we should revert what Markus started.   Both ways of
> > > referencing QEMU headers have downsides, but I think "..." has fewer
> > > downsides that "<">.
> > 
> > Could you please explain what the advantage of "" is?
> > It seems to be gone since we moved headers away from
> > source.
> 
> We moved *some* headers into the include/ directory tree.
> 
> I still count 650+ headers which are alongside the .c files.
> 
> Regards,
> Daniel

So for these, we should use "".  None of these are generated files though.

> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 02:12:24PM +0200, Michael S. Tsirkin wrote:
> On Tue, Mar 20, 2018 at 09:44:06AM +, Daniel P. Berrangé wrote:
> > On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote:
> > > Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> > > > QEMU coding style at the moment asks for all non-system
> > > > include files to be used with #include "foo.h".
> > > > However this rule actually does not make sense and
> > > > creates issues for when the included file is generated.
> > > 
> > > If you change that, we can have issue when a system include has the same
> > > name as our local include. With "", system header are taken first.
> > 
> > > > In C, include "file" means look in current directory,
> > > > then on include search path. Current directory here
> > > > means the source file directory.
> > > > By comparison include  means look on include search path.
> > > 
> > > Not exactly, there is the notion of "system header" too.
> > > 
> > > https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
> > > 
> > > #include 
> > > This variant is used for system header files. It searches for a file
> > > named file in a standard list of system directories. You can prepend
> > > directories to this list with the -I option (see Invocation).
> > > 
> > > #include "file"
> > > This variant is used for header files of your own program. It searches
> > > for a file named file first in the directory containing the current
> > > file, then in the quote directories and then the same directories used
> > > for . You can prepend directories to the list of quote directories
> > > with the -iquote option.
> > > 
> > > > As generated files are not in the search directory (unless the build
> > > > directory happens to match the source directory), it does not make sense
> > > > to include them with "" - doing so is merely more work for preprocessor
> > > > and a source or errors if a stale file happens to exist in the source
> > > > directory.
> > > 
> > > I agree there is a problem with stale files. But linux, for instance,
> > > asks for a "make mrproper" to avoid this.
> > 
> > We can follow what autoconf does, and add a check to configure to see if
> > there are generated files left in the source dir, when configuring with
> > builddir != srcdir, and exit with error, telling user to clean their
> > src dir first.
> > 
> > > > This changes include directives for all generated files, across the
> > > > tree. The idea is to avoid sending a huge amount of email.  But when
> > > > merging, the changes will be split with one commit per file, e.g. for
> > > > ease of bisect in case of build failures, and to ease merging.
> > > > 
> > > > Note that should some generated files be missed by this tree-wide
> > > > refactoring, it isn't a big deal - this merely maintains the status quo,
> > > > and this can be addressed by a separate patch on top.
> > > > 
> > > > Signed-off-by: Michael S. Tsirkin 
> > > 
> > > I think your idea conflicts with what Markus has started to do:
> > 
> > Yes, I don't think we should revert what Markus started.   Both ways of
> > referencing QEMU headers have downsides, but I think "..." has fewer
> > downsides that "<">.
> 
> Could you please explain what the advantage of "" is?
> It seems to be gone since we moved headers away from
> source.

We moved *some* headers into the include/ directory tree.

I still count 650+ headers which are alongside the .c files.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 09:44:06AM +, Daniel P. Berrangé wrote:
> On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote:
> > Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> > > QEMU coding style at the moment asks for all non-system
> > > include files to be used with #include "foo.h".
> > > However this rule actually does not make sense and
> > > creates issues for when the included file is generated.
> > 
> > If you change that, we can have issue when a system include has the same
> > name as our local include. With "", system header are taken first.
> 
> > > In C, include "file" means look in current directory,
> > > then on include search path. Current directory here
> > > means the source file directory.
> > > By comparison include  means look on include search path.
> > 
> > Not exactly, there is the notion of "system header" too.
> > 
> > https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
> > 
> > #include 
> > This variant is used for system header files. It searches for a file
> > named file in a standard list of system directories. You can prepend
> > directories to this list with the -I option (see Invocation).
> > 
> > #include "file"
> > This variant is used for header files of your own program. It searches
> > for a file named file first in the directory containing the current
> > file, then in the quote directories and then the same directories used
> > for . You can prepend directories to the list of quote directories
> > with the -iquote option.
> > 
> > > As generated files are not in the search directory (unless the build
> > > directory happens to match the source directory), it does not make sense
> > > to include them with "" - doing so is merely more work for preprocessor
> > > and a source or errors if a stale file happens to exist in the source
> > > directory.
> > 
> > I agree there is a problem with stale files. But linux, for instance,
> > asks for a "make mrproper" to avoid this.
> 
> We can follow what autoconf does, and add a check to configure to see if
> there are generated files left in the source dir, when configuring with
> builddir != srcdir, and exit with error, telling user to clean their
> src dir first.
> 
> > > This changes include directives for all generated files, across the
> > > tree. The idea is to avoid sending a huge amount of email.  But when
> > > merging, the changes will be split with one commit per file, e.g. for
> > > ease of bisect in case of build failures, and to ease merging.
> > > 
> > > Note that should some generated files be missed by this tree-wide
> > > refactoring, it isn't a big deal - this merely maintains the status quo,
> > > and this can be addressed by a separate patch on top.
> > > 
> > > Signed-off-by: Michael S. Tsirkin 
> > 
> > I think your idea conflicts with what Markus has started to do:
> 
> Yes, I don't think we should revert what Markus started.   Both ways of
> referencing QEMU headers have downsides, but I think "..." has fewer
> downsides that "<">.

Could you please explain what the advantage of "" is?
It seems to be gone since we moved headers away from
source.

> The problem Michael is tackling should be pretty rare, because moist
> developers aren't frequently switching between srcdir==builddir and
> srcdir!=biulddir setups - they have their preference for which to use
> and stick with it. As long as we get ./configure to warn about the
> dirty srcdir it should be good enough
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote:
> Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> > QEMU coding style at the moment asks for all non-system
> > include files to be used with #include "foo.h".
> > However this rule actually does not make sense and
> > creates issues for when the included file is generated.
> 
> If you change that, we can have issue when a system include has the same
> name as our local include. With "", system header are taken first.

Are you sure? I just tested and that is not the case with
either gcc or clang.

> > In C, include "file" means look in current directory,
> > then on include search path. Current directory here
> > means the source file directory.
> > By comparison include  means look on include search path.
> 
> Not exactly, there is the notion of "system header" too.
> 
> https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
> 
> #include 
> This variant is used for system header files. It searches for a file
> named file in a standard list of system directories. You can prepend
> directories to this list with the -I option (see Invocation).

This is exactly what we do.

> #include "file"
> This variant is used for header files of your own program. It searches
> for a file named file first in the directory containing the current
> file, then in the quote directories and then the same directories used
> for . You can prepend directories to the list of quote directories
> with the -iquote option.

Since we do not use -iquote, "" just adds the current directory.


> > As generated files are not in the search directory (unless the build
> > directory happens to match the source directory), it does not make sense
> > to include them with "" - doing so is merely more work for preprocessor
> > and a source or errors if a stale file happens to exist in the source
> > directory.
> 
> I agree there is a problem with stale files. But linux, for instance,
> asks for a "make mrproper" to avoid this.

Using <> we avoid the problem completely and create slightly
less work for the preprocessor.

> > This changes include directives for all generated files, across the
> > tree. The idea is to avoid sending a huge amount of email.  But when
> > merging, the changes will be split with one commit per file, e.g. for
> > ease of bisect in case of build failures, and to ease merging.
> > 
> > Note that should some generated files be missed by this tree-wide
> > refactoring, it isn't a big deal - this merely maintains the status quo,
> > and this can be addressed by a separate patch on top.
> > 
> > Signed-off-by: Michael S. Tsirkin 
> 
> I think your idea conflicts with what Markus has started to do:
> 
> commit d8e39b70625d4ba1e998439d1a077b4b978930e7
> Author: Markus Armbruster 
> Date:   Thu Feb 1 12:18:28 2018 +0100
> 
> 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.
> 
> Delete inclusions of "string.h" and "strings.h" instead of fixing them
> to  and , because we always include these via
> osdep.h.
> 
> Put the cleaned up system header includes first.
> 
> While there, separate #include from file comment with exactly one
> blank line.
> 
> commit a9c94277f07d19d3eb14f199c3e93491aa3eae0e
> Author: Markus Armbruster 
> Date:   Wed Jun 22 19:11:19 2016 +0200
> 
> Use #include "..." for our own headers, <...> for others
> 
> Tracked down with an ugly, brittle and probably buggy Perl script.
> 
> Also move includes converted to <...> up so they get included before
> ours where that's obviously okay.
> 
> Thanks,
> Laurent

I suspect we previously actually did have headers in the
same directory as source so it was somewhat helpful.
They all have been moved out to include now.

-- 
MST

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 10:27:19AM +, Daniel P. Berrangé wrote:
> On Tue, Mar 20, 2018 at 10:01:24AM +, Peter Maydell wrote:
> > On 20 March 2018 at 09:44, Daniel P. Berrangé  wrote:
> > > We can follow what autoconf does, and add a check to configure to see if
> > > there are generated files left in the source dir, when configuring with
> > > builddir != srcdir, and exit with error, telling user to clean their
> > > src dir first.
> > 
> > We already do this in our makefile...it just doesn't check every
> > single generated file.
> 
> Ah yes, indeed:
> 
> $ make
> Makefile:59: *** This is an out of tree build but your source tree
> (/home/berrange/src/virt/qemu) seems to have been used for an in-tree
> build. You can fix this by running "make distclean && rm -rf *-linux-user
>  *-softmmu" in your source tree.  Stop.
> 
> 
> It is checking for existance of config-host.mak.
> 
> We have a convenient list of generated files in $(GENERATED_FILES), so
> I wonder if there's a practical way to check all of those too.
> 
> Regards,
> Daniel

With my patch presence of these files mostly stops being an issue.

> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 10:01:24AM +, Peter Maydell wrote:
> On 20 March 2018 at 09:44, Daniel P. Berrangé  wrote:
> > We can follow what autoconf does, and add a check to configure to see if
> > there are generated files left in the source dir, when configuring with
> > builddir != srcdir, and exit with error, telling user to clean their
> > src dir first.
> 
> We already do this in our makefile...it just doesn't check every
> single generated file.

Ah yes, indeed:

$ make
Makefile:59: *** This is an out of tree build but your source tree
(/home/berrange/src/virt/qemu) seems to have been used for an in-tree
build. You can fix this by running "make distclean && rm -rf *-linux-user
 *-softmmu" in your source tree.  Stop.


It is checking for existance of config-host.mak.

We have a convenient list of generated files in $(GENERATED_FILES), so
I wonder if there's a practical way to check all of those too.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Peter Maydell]

On 20 March 2018 at 09:44, Daniel P. Berrangé  wrote:
> We can follow what autoconf does, and add a check to configure to see if
> there are generated files left in the source dir, when configuring with
> builddir != srcdir, and exit with error, telling user to clean their
> src dir first.

We already do this in our makefile...it just doesn't check every
single generated file.

thanks
-- PMM

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote:
> Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> > QEMU coding style at the moment asks for all non-system
> > include files to be used with #include "foo.h".
> > However this rule actually does not make sense and
> > creates issues for when the included file is generated.
> 
> If you change that, we can have issue when a system include has the same
> name as our local include. With "", system header are taken first.

> > In C, include "file" means look in current directory,
> > then on include search path. Current directory here
> > means the source file directory.
> > By comparison include  means look on include search path.
> 
> Not exactly, there is the notion of "system header" too.
> 
> https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
> 
> #include 
> This variant is used for system header files. It searches for a file
> named file in a standard list of system directories. You can prepend
> directories to this list with the -I option (see Invocation).
> 
> #include "file"
> This variant is used for header files of your own program. It searches
> for a file named file first in the directory containing the current
> file, then in the quote directories and then the same directories used
> for . You can prepend directories to the list of quote directories
> with the -iquote option.
> 
> > As generated files are not in the search directory (unless the build
> > directory happens to match the source directory), it does not make sense
> > to include them with "" - doing so is merely more work for preprocessor
> > and a source or errors if a stale file happens to exist in the source
> > directory.
> 
> I agree there is a problem with stale files. But linux, for instance,
> asks for a "make mrproper" to avoid this.

We can follow what autoconf does, and add a check to configure to see if
there are generated files left in the source dir, when configuring with
builddir != srcdir, and exit with error, telling user to clean their
src dir first.

> > This changes include directives for all generated files, across the
> > tree. The idea is to avoid sending a huge amount of email.  But when
> > merging, the changes will be split with one commit per file, e.g. for
> > ease of bisect in case of build failures, and to ease merging.
> > 
> > Note that should some generated files be missed by this tree-wide
> > refactoring, it isn't a big deal - this merely maintains the status quo,
> > and this can be addressed by a separate patch on top.
> > 
> > Signed-off-by: Michael S. Tsirkin 
> 
> I think your idea conflicts with what Markus has started to do:

Yes, I don't think we should revert what Markus started.   Both ways of
referencing QEMU headers have downsides, but I think "..." has fewer
downsides that "<">.

The problem Michael is tackling should be pretty rare, because moist
developers aren't frequently switching between srcdir==builddir and
srcdir!=biulddir setups - they have their preference for which to use
and stick with it. As long as we get ./configure to warn about the
dirty srcdir it should be good enough

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Laurent Vivier]

Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> QEMU coding style at the moment asks for all non-system
> include files to be used with #include "foo.h".
> However this rule actually does not make sense and
> creates issues for when the included file is generated.

If you change that, we can have issue when a system include has the same
name as our local include. With "", system header are taken first.

> In C, include "file" means look in current directory,
> then on include search path. Current directory here
> means the source file directory.
> By comparison include  means look on include search path.

Not exactly, there is the notion of "system header" too.

https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html

#include 
This variant is used for system header files. It searches for a file
named file in a standard list of system directories. You can prepend
directories to this list with the -I option (see Invocation).

#include "file"
This variant is used for header files of your own program. It searches
for a file named file first in the directory containing the current
file, then in the quote directories and then the same directories used
for . You can prepend directories to the list of quote directories
with the -iquote option.

> As generated files are not in the search directory (unless the build
> directory happens to match the source directory), it does not make sense
> to include them with "" - doing so is merely more work for preprocessor
> and a source or errors if a stale file happens to exist in the source
> directory.

I agree there is a problem with stale files. But linux, for instance,
asks for a "make mrproper" to avoid this.

> This changes include directives for all generated files, across the
> tree. The idea is to avoid sending a huge amount of email.  But when
> merging, the changes will be split with one commit per file, e.g. for
> ease of bisect in case of build failures, and to ease merging.
> 
> Note that should some generated files be missed by this tree-wide
> refactoring, it isn't a big deal - this merely maintains the status quo,
> and this can be addressed by a separate patch on top.
> 
> Signed-off-by: Michael S. Tsirkin 

I think your idea conflicts with what Markus has started to do:

commit d8e39b70625d4ba1e998439d1a077b4b978930e7
Author: Markus Armbruster 
Date:   Thu Feb 1 12:18:28 2018 +0100

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.

Delete inclusions of "string.h" and "strings.h" instead of fixing them
to  and , because we always include these via
osdep.h.

Put the cleaned up system header includes first.

While there, separate #include from file comment with exactly one
blank line.

commit a9c94277f07d19d3eb14f199c3e93491aa3eae0e
Author: Markus Armbruster 
Date:   Wed Jun 22 19:11:19 2016 +0200

Use #include "..." for our own headers, <...> for others

Tracked down with an ugly, brittle and probably buggy Perl script.

Also move includes converted to <...> up so they get included before
ours where that's obviously okay.

Thanks,
Laurent

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel