Re: [Qemu-block] [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 "<FILE>", 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 <file> 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 <file>
> 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 <file>. 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 <address@hidden>
> 
> I think your idea conflicts with what Markus has started to do:
> 
> commit d8e39b70625d4ba1e998439d1a077b4b978930e7
> Author: Markus Armbruster <address@hidden>
> 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 <string.h> and <strings.h>, 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 <address@hidden>
> 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