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.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
scripts/changeheaders.pl | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
create mode 100755 scripts/changeheaders.pl
diff --git a/scripts/changeheaders.pl b/scripts/changeheaders.pl
new file mode 100755
index 0000000..22bd5b8
--- /dev/null
+++ b/scripts/changeheaders.pl
@@ -0,0 +1,20 @@
+#!/usr/bin/perl -pi
+
+if (m@^\s*#include\s+"([^"+]"@o) {
+ next;
+}
+
+my $hdr = $1;
+my $file = $ARGV;
+$file =~ s@/[^/]+$@@g;
+$file .= $hdr;
+
+if (-e $file) {
+ next;
+}
+
+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) {
+}
--
MST
On 21/03/2018 15:46, Michael S. Tsirkin wrote:
> +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
On Wed, Mar 21, 2018 at 04:04:29PM +0100, Paolo Bonzini wrote:
> On 21/03/2018 15:46, Michael S. Tsirkin wrote:
> > +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?
include/qemu -> include/qemu/common
include/* -> include/qemu/*
Thus one uses any qemu headers with
#include <qemu/....>
we can do the conversion gradually and avoid a flag day
with some use of softlinks.
> Also, s{}{} and m{} are a bit more readable.
>
> Paolo
Thanks, will use.
--
MST
On 21/03/2018 16:11, Michael S. Tsirkin wrote: >> Can you explain the changes in the source tree layout? > include/qemu -> include/qemu/common > include/* -> include/qemu/* Ok, then perhaps "util" instead of common would match the source layout more. Paolo > Thus one uses any qemu headers with > > #include <qemu/....>
On Wed, Mar 21, 2018 at 04:46:32PM +0200, Michael S. Tsirkin wrote: > 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 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. > 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. > 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. 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 -- |: 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 :|
On Wed, Mar 21, 2018 at 03:19:22PM +0000, Daniel P. Berrangé wrote: > On Wed, Mar 21, 2018 at 04:46:32PM +0200, Michael S. Tsirkin wrote: > > 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 > > 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. > This is exactly what this patch proposes, with a uniform scheme: start everything with qemu/. > > > 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. We can not know that there are no system headers that start with block/ on any current or future systems. > > 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. > > 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. I think you are mistaken on the last point: 1. Everything will be under qemu/ so we never clash with a system file 2. A local stale file anywhere in source directory is completely ignored since source is not on -I path. I hope this clarifies things. > > 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 :|
On Wed, Mar 21, 2018 at 05:39:48PM +0200, Michael S. Tsirkin wrote: > On Wed, Mar 21, 2018 at 03:19:22PM +0000, Daniel P. Berrangé wrote: > > On Wed, Mar 21, 2018 at 04:46:32PM +0200, Michael S. Tsirkin wrote: > > > 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 > > > > 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. > > > > This is exactly what this patch proposes, with a uniform scheme: > start everything with qemu/. > > > > > > 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. > > We can not know that there are no system headers that start with block/ on > any current or future systems. Ah true, good point. I guess that's where the benefit of -iquote comes into play. 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 :|
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. > 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
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. It's all still very much a non-standard convention and so less robust than prefixing file name with a project-specifix prefix. > > 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. 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. -- MST
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
On Wed, Mar 21, 2018 at 05:22:03PM +0100, Kevin Wolf wrote: > > 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? > > > > 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. Not if we have some automatic tools to catch violators. > > 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 the rule is that "" is only for files in the current directory then we can easily code up a checkpatch script to catch violators. > 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 If my proposal to prefix all include directories with qemu/ is accepted, then we can solve the stale file problem by prohibiting a directory named qemu everywhere in source. -- MST
On 22/03/2018 20:29, Michael S. Tsirkin wrote: > On Wed, Mar 21, 2018 at 05:22:03PM +0100, Kevin Wolf wrote: >>> 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? GCC - https://github.com/gcc-mirror/gcc/blob/master/gcc/reload.c Libvirt - https://github.com/libvirt/libvirt/blob/master/src/util/virprocess.c SDL - https://github.com/SDL-mirror/SDL/blob/master/src/core/unix/SDL_poll.c Anything but Linux really. I find <qemu/foo/bar.h> verbose and unnecessary. The only advantage of your proposal is that files included from the source directory would be clearly noticeable. That said, it's easy to add a checkpatch.pl rule that detects when ".../..." is used on a file not under include/. Paolo
© 2016 - 2025 Red Hat, Inc.