Re: [Qemu-arm] [PATCH v2] qemu: replace "" with <> in headers

[Kevin Wolf]

Am 21.03.2018 um 16:58 hat Michael S. Tsirkin geschrieben:
> On Wed, Mar 21, 2018 at 04:34:39PM +0100, Kevin Wolf wrote:
> > Am 21.03.2018 um 15:46 hat Michael S. Tsirkin geschrieben:
> > > Our current scheme is to use
> > >  #include ""
> > > for internal headers, and
> > >  #include <>
> > > for external ones.
> > > 
> > > Unfortunately this is not based on compiler support: from C point of
> > > view, the "" form merely looks up headers in the current directory
> > > and then falls back on <> directories.
> > > 
> > > Thus, for example, a system header trace.h - should it be present - will
> > > conflict with our local trace.h
> > 
> > You're right that there is a conflict, even though only in one
> > direction: "trace.h" is unambiguously the local trace.h in our source
> > tree, but <trace.h> refers to the same local header rather than the
> > system header as you would expect.
> > 
> > An easy way to resolve this conflict would be using -iquote rather than
> > -I for directories in the source tree, so that <trace.h> unambiguously
> > refers to the system header and "trace.h" unambiguously refers to the
> > QEMU header.
> 
> I posted patches to that effect for 2.12.

Ah, I missed those. That's good (and imho enough).

> It's all still very much a non-standard convention and so less robust
> than prefixing file name with a project-specifix prefix.

I've always had the impression that it's by far the most common
convention, to the point that I'd blindly assume it when joining a new
project.

> > > As another example of problems, a header by the same name in the source
> > > directory will always be picked up first - before any headers in
> > > the include directory.
> > > 
> > > Let's change the scheme: make sure all headers that are not
> > > in the source directory are included through a path
> > > starting with qemu/ , thus:
> > > 
> > >  #include <>
> > > 
> > > headers in the same directory as source are included with
> > > 
> > >  #include ""
> > > 
> > > as per standard.
> > > 
> > > This (untested) patch is just to start the discussion and does not
> > > change all of the codebase. If there's agreement, this will be
> > > run on all code to converting code to this scheme.
> > 
> > Renaming files is always painful. If that's the fix, the cure might be
> > worse than the disease. As far as I know, the conflict is only
> > theoretical, so in that case I'd say: If it ain't broke, don't fix it.
> > 
> > Kevin
> 
> It's broke I think, it's very hard for new people to contribute to QEMU.
> Look e.g. at rdma which all has messed up includes - and that's from an
> experienced conributor who just isn't an experienced maintainer.

I don't think the problem is that the convention is hard to apply (it's
definitely not). It's knowing about the convention. This problem isn't
going away by switching to a different, less common convention. We're
only going to see more offenders then.

> Amount of time spent on teaching new people trivia about our
> conventions just isn't funny. They should be self-documenting
> and violations should cause the build to fail.

Yes, but your proposal doesn't achieve this. You can still use
"qemu/foo.h" instead of <qemu/foo.h> and it will build successfully.
That's something we can't change, as far as I know, because the include
path for "foo.h" is always a superset of <foo.h>.

If anything, this means that we should prefer "foo.h" for local headers
(i.e. the way it currently is) because we can let the compiler enforce
it: <foo.h> for "foo.h" can become a build error, and does so with your
-iquote patch, but the other way round doesn't work.

Then it's only system headers that you can possibly get wrong, but for
those everyone should be used to using <foo.h> anyway.

Kevin