Open GoogleCodeExporter opened 9 years ago
Here is a small example showing the issue. There we see iwyu suggest removing a
useless include (good!) but also removing a single good include to replace it
by five others!
It might also be problematic to follow the suggestion, since the code wouldn't
#include <stdio.h> anymore... but that might be another bug!
Original comment by julien.puydt
on 8 Sep 2013 at 12:25
Attachments:
To be more explicit about the example:
- there are five include files biglib/feature1.h to biglib/feature5.h, each
defining a #FEATUREN printf("Feature N\n")
- there is an empy useless.h
- there is a biglib.h which #includes <stdio.h> and the five biglib/featureN.h
- test.c #includes useless.h and biglist.h
If you ask for iwyu's opinion, it will ask to ditch the two #includes, and
replace them with five ones to the biglib/featureN.h, so this one example:
- shows that iwyu should give an option to just suggest to remove the useless
includes (and not go on suggesting a 1-to-5 change) ; this is a wishlist-level
problem ;
- shows that iwyu will happily suggest removing a much-needed inclusion of
stdio.h, and following its advice will break the compilation ; this is a
grave-level problem.
Original comment by julien.puydt
on 8 Sep 2013 at 12:38
It looks like IWYU is doing what it's supposed to.
If you want to solve the facade/detail problem (your wishlist-level one), you
can use IWYU pragmas
(https://code.google.com/p/include-what-you-use/wiki/IWYUPragmas) or IWYU
mapping files
(https://code.google.com/p/include-what-you-use/wiki/IWYUMappings).
As for the stdio.h problem, I think IWYU should be suggesting to move #include
<stdio.h> to the individual feature headers; they are the ones using printf,
and they are essentially re-exporting printf under a different name. If it
doesn't, I would consider that a bug.
Original comment by kim.gras...@gmail.com
on 8 Sep 2013 at 2:56
It doesn't suggest to move the #include <stdio.h> into the individual headers ;
in fact it doesn't mention it at all... but the suggestions have as consequence
to make it disappear, which is pretty bad.
The mappings feature looks like something I could use ; it will be annoying
that the include directives don't look like it's possible/easy to use
wildcards, but I think some python script magic will help there.
Original comment by julien.puydt
on 10 Sep 2013 at 6:51
I've forgotten to document it, but you should be able to use @-prefixed regexes
in the from-mappings, e.g.
{ include: ["@<internal/.*>", "private", "<public>", "public"] }
Not sure how far that gets you.
Original comment by kim.gras...@gmail.com
on 10 Sep 2013 at 7:12
That doesn't bring me as far as I wanted: I created an ekiga.imp which
references glib.imp and gtk.imp, where I use regexes as explained. But the
project is autotools-based ; I was testing things like this:
make CC=iwyu CXX=iwyu V=1 > /tmp/iwyu.log 2>&1
but if I try to add the "--mapping_file=..." in there (either in CC/CXX or
through CFLAGS/CXXFLAGS), the autotools take the argument as being for them!
Is there a way I can send an option to iwyu directly (either an environment
variable or a configuration file)?
Original comment by julien.puydt
on 11 Sep 2013 at 6:45
I don't know anything about autotools, but there must be a way to funnel
arguments through to $CXX, right?
Unix is not my primary platform, but I'd try to add mappings to CXXFLAGS. Don't
forget to prefix with -Xiwyu, e.g.
CXXFLAGS=-Xiwyu --mapping_file=...
Sorry I can't help more, maybe there are other autotools users out there.
Original comment by kim.gras...@gmail.com
on 11 Sep 2013 at 6:53
Ah, indeed -Xiwyu was missing in my tests. Now I get it to run correctly, but:
{ include: ["@<glib/.*>", private, "<glib.h>", public]},
still makes iwyu suggest to add "glib/gmacros.h" and "glib/gtypes.h", and
remove <glib.h>...
Original comment by julien.puydt
on 11 Sep 2013 at 7:20
Note: if you want to document better how one can use iwyu with autotools, I
compile using:
make CFLAGS="-Xiwyu --mapping_file=/home/jpuydt/Logiciels/Ekiga/ekiga.imp" CXXFLAGS="-Xiwyu --mapping_file=/home/jpuydt/Logiciels/Ekiga/ekiga.imp" CC=iwyu CXX=iwyu V=1 -k > /tmp/iwyu.log 2>&1
Original comment by julien.puydt
on 11 Sep 2013 at 7:22
The quotes are significant (though I'm not sure why, that decision predates me
:-)), so you may have better luck with:
{ include: ["@\"glib/.*\"", private, "<glib.h>", public]},
or whatever the escape syntax for quotes is in JSON.
Thanks for the concrete example of autotools use, I'll try and add it to the
HowToUse wiki page.
Original comment by kim.gras...@gmail.com
on 11 Sep 2013 at 8:06
That's better indeed, but now I find situations where iwyu tells me to remove a
#include, and tells me it is good where it is!
See:
../../ekiga.git/lib/platform/platform.c should add these lines:
#include <stddef.h> // for NULL
../../ekiga.git/lib/platform/platform.c should remove these lines:
- #include <gtk/gtk.h> // lines 38-38
The full include-list for ../../ekiga.git/lib/platform/platform.c:
#include "platform.h"
#include <gtk/gtk.h> // for gtk_show_uri, etc
#include <stddef.h> // for NULL
Original comment by julien.puydt
on 11 Sep 2013 at 11:56
Without more context, I can imagine two things:
1. gtk/gtk.h has been included twice, and IWYU removes one of them
2. "// for ..." comments are added, and therefore the #include is rewritten
I don't think IWYU does (2), so I'm leaning towards (1).
Original comment by kim.gras...@gmail.com
on 11 Sep 2013 at 6:50
I just realized I hadn't tested your attached example, I was disracted by the
follow-on discussion.
> - shows that iwyu should give an option to just suggest to remove the useless
> includes (and not go on suggesting a 1-to-5 change) ; this is a wishlist-level
> problem ;
I think we covered this, the inlining of biglib feature headers is by design.
Without metadata for a library, we don't know which headers are private and
which are public. Mappings should help here (even if they're inconvenient.)
> - shows that iwyu will happily suggest removing a much-needed inclusion
> of stdio.h, and following its advice will break the compilation ; this
> is a grave-level problem.
Ah, so IWYU losing stdio is a consequence of inlining the feature headers?
I think that's because IWYU does not modify headers by default (other than the
'associated' header, i.e. the one with the same basename as the .c file being
processed.)
You can let IWYU know which headers it it allows to change in its analysis
using the --check_also switch:
$ include-what-you-use.exe test.c -I. -Ibiglib -Xiwyu
--check_also=biglib/feature*.h
When I add that, IWYU suggests that #include <stdio.h> be added in all feature
headers, and then inlines biglib.h and removes useless.h.
That's one of the challenges with IWYU, that it can't do a big-bang fix of an
entire codebase. It works translation unit by translation unit, and shared
headers are not implicitly fixed (because it would run the risk of conflicts).
The user has to explicitly --check_also in the context of one translation unit.
IWYU for larger projects would have to run several passes to fix #include
dependencies inside-out.
Original comment by kim.gras...@gmail.com
on 6 Oct 2013 at 7:36
I'd love to hear more about the problem in comment #11, though.
Can you run IWYU for platform.c with -Xiwyu --verbose=7 and upload the log
output?
Original comment by kim.gras...@gmail.com
on 6 Oct 2013 at 7:39
Original issue reported on code.google.com by
julien.puydt
on 8 Sep 2013 at 11:29