qemu: replace "" with <> in headers

scripts/changeheaders.pl line 20

Quoted 5 lines · Michael S. Tsirkin

scripts/changeheaders.pl new file +5 -0

+if (m@^\s*#include\s+"qemu/@o) {
+    s@^(\s*#include\s+)"qemu/([^"]+)"(.*)$@$1<qemu/common/$2>$3@o) {
+} else {
+    s@^(\s*#include\s+)"([^"]+)"(.*)$@$1<qemu/$2>$3@o) {
+}

Can you explain the changes in the source tree layout?

Also, s{}{} and m{} are a bit more readable.

Paolo

Quoted 1 line

Can you explain the changes in the source tree layout?

include/qemu -> include/qemu/common
include/* -> include/qemu/*

Thus one uses any qemu headers with

#include <qemu/....>

scripts/changeheaders.pl line 20

Quoted 5 lines · Michael S. Tsirkin

scripts/changeheaders.pl new file +5 -0

+if (m@^\s*#include\s+"qemu/@o) {
+    s@^(\s*#include\s+)"qemu/([^"]+)"(.*)$@$1<qemu/common/$2>$3@o) {
+} else {
+    s@^(\s*#include\s+)"([^"]+)"(.*)$@$1<qemu/$2>$3@o) {
+}

Can you explain the changes in the source tree layout?

Also, s{}{} and m{} are a bit more readable.

Paolo

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

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.

Quoted 6 lines · Michael S. Tsirkin

Our current scheme is to use
#include ""
for internal headers, and
#include <>
for external ones.

Quoted 6 lines

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

If our local "trace.h" is in the current directory, then using "" is right and you can still use <trace.h> to get the system version.

If our local trace.h is in include/ top level, then it is going to block use of the system trace.h regardless of whether we use <> or ""

Fortunately our include/ tree uses sub-dirs, so we would typically use #include "$subdir/trace.h" and #include <trace.h> would still find the system header.
We just have to be careful we don't add stuff at the top level of our include/ dir with names that are liable to clash. This might suggest renaming include/elf.h to include/qemu/elf.h, or just moving elf.h to the qemu/ subdirectory. Likewise include/glib-compat.h might be better moved to qemu/ subdirectory.

Quoted 4 lines

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.

There's only a couple of headers in the top level of our include/ directory - everything else is pulled in with a named path eg #include "block/block_int.h", so that would not conflict with reference to a bare #include "block_int.h" from the current directory.

Quoted 10 lines

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 ""

Quoted 1 line

as per standard.

As stated before, I consider this a step backwards - it is a good clear standard to use "" for project local includes and <> for 3rd party / system includes IMHO. The change doesn't do anything beneficial for the two scenarios described above AFAICT.

Regards,
Daniel

Signature · 3 lines

|: 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 :|

Quoted 20 lines · Daniel P. Berrangé

Quoted 6 lines · Michael S. Tsirkin

Our current scheme is to use
#include ""
for internal headers, and
#include <>
for external ones.

Quoted 6 lines

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

If our local "trace.h" is in the current directory, then using "" is right and you can still use <trace.h> to get the system version.

If our local trace.h is in include/ top level, then it is going to block use of the system trace.h regardless of whether we use <> or ""

Quoted 9 lines

Fortunately our include/ tree uses sub-dirs, so we would typically use #include "$subdir/trace.h" and #include <trace.h> would still find the system header.
We just have to be careful we don't add stuff at the top level of our include/ dir with names that are liable to clash. This might suggest renaming include/elf.h to include/qemu/elf.h, or just moving elf.h to the qemu/ subdirectory. Likewise include/glib-compat.h might be better moved to qemu/ subdirectory.

This is exactly what this patch proposes, with a uniform scheme:
start everything with qemu/.

Quoted 5 lines

Quoted 4 lines

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.

Quoted 4 lines

There's only a couple of headers in the top level of our include/ directory - everything else is pulled in with a named path eg #include "block/block_int.h", so that would not conflict with reference to a bare #include "block_int.h" from the current directory.

We can not know that there are no system headers that start with block/ on any current or future systems.

Quoted 20 lines · Daniel P. Berrangé

Quoted 6 lines · Michael S. Tsirkin

Our current scheme is to use
#include ""
for internal headers, and
#include <>
for external ones.

Quoted 6 lines

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

If our local "trace.h" is in the current directory, then using "" is right and you can still use <trace.h> to get the system version.

If our local trace.h is in include/ top level, then it is going to block use of the system trace.h regardless of whether we use <> or ""

Quoted 9 lines

Fortunately our include/ tree uses sub-dirs, so we would typically use #include "$subdir/trace.h" and #include <trace.h> would still find the system header.
We just have to be careful we don't add stuff at the top level of our include/ dir with names that are liable to clash. This might suggest renaming include/elf.h to include/qemu/elf.h, or just moving elf.h to the qemu/ subdirectory. Likewise include/glib-compat.h might be better moved to qemu/ subdirectory.

This is exactly what this patch proposes, with a uniform scheme:
start everything with qemu/.

Quoted 5 lines

Quoted 4 lines

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.

Quoted 4 lines

There's only a couple of headers in the top level of our include/ directory - everything else is pulled in with a named path eg #include "block/block_int.h", so that would not conflict with reference to a bare #include "block_int.h" from the current directory.

We can not know that there are no system headers that start with block/ on any current or future systems.

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

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.

Am 21.03.2018 um 15:46 hat Michael S. Tsirkin geschrieben:

Quoted 6 lines

Our current scheme is to use
#include ""
for internal headers, and
#include <>
for external ones.

Quoted 6 lines

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.

Quoted 12 lines

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

Quoted 7 lines

#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

Quoted 19 lines

Am 21.03.2018 um 15:46 hat Michael S. Tsirkin geschrieben:

Quoted 6 lines

Our current scheme is to use
#include ""
for internal headers, and
#include <>
for external ones.

Quoted 6 lines

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.

Quoted 4 lines

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.

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

Quoted 24 lines

Quoted 12 lines

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

Quoted 7 lines

#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.

Quoted 1 line

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.

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.

Quoted 2 lines

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.

Quoted 26 lines

Quoted 25 lines

Quoted 12 lines

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

Quoted 7 lines

#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

Quoted 3 lines

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.

Quoted 3 lines

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

Quoted 5 lines · Kevin Wolf

Quoted 2 lines

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.

Any examples?

Quoted 5 lines · Kevin Wolf

Quoted 2 lines

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.

Any examples?

Quoted 2 lines

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.

Quoted 26 lines

Quoted 25 lines

Quoted 12 lines

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

Quoted 7 lines

#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

Quoted 3 lines

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.

Quoted 3 lines

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

Am 21.03.2018 um 15:46 hat Michael S. Tsirkin geschrieben:

Quoted 6 lines

Our current scheme is to use
#include ""
for internal headers, and
#include <>
for external ones.

Quoted 6 lines

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.

Quoted 12 lines

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

Quoted 7 lines

#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