Closed GoogleCodeExporter closed 9 years ago
New output
Original comment by alexey.ivanes@gmail.com
on 31 Jan 2013 at 12:56
Attachments:
IWYU also difficult to use with Qt projects.
For
#include <QString>
suggests
#include "qstring.h"
Original comment by alexey.ivanes@gmail.com
on 31 Jan 2013 at 1:01
Also in Qt no need to analyze moc_*.cpp files
Original comment by alexey.ivanes@gmail.com
on 31 Jan 2013 at 1:09
Please open a separate issue for each problem, thanks! Comment #2 and comment
#3 sound like they should be new issues, and you can delete the comments
afterwards.
We assume iwyu.gcc.imp is located next to the include-what-you-use binary. So,
if you're running IWYU out of the Clang build tree, please copy *.imp over from
the source directory together with the executable.
Unfortunately I don't know my way around make install or CMake's equivalent to
have them published there automatically. Patches most welcome!
Original comment by kim.gras...@gmail.com
on 31 Jan 2013 at 5:59
(I posted to the list about how I dealt with Qt header mappings:
https://groups.google.com/forum/?fromgroups=#!topic/include-what-you-use/8ABKFKe
t6kA)
Original comment by zaheer.c...@gmail.com
on 31 Jan 2013 at 11:14
I haven't tested this, but I think the files can be installed by adding this to
CMakeLists.txt:
install(
FILES
gcc.libc.imp
gcc.stl.headers.imp
gcc.symbols.imp
google.imp
iwyu.gcc.imp
DESTINATION bin)
I don't know much about the Makefiles, but here is a rule which installs
Clang's header files:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Makefile?view=markup
Original comment by zaheer.c...@gmail.com
on 31 Jan 2013 at 11:28
>DESTINATION bin)
after "make install" imp files into /usr/bin?
Original comment by alexey.ivanes@gmail.com
on 31 Jan 2013 at 11:42
Zaheer, thanks for the hints! I was able to start from that and work my way to
a patch to fix the CMake build to;
a) install *.imp alongside the include-what-you-use executable
b) copy *.imp alongside the built include-what-you-use executable after every
build
I still haven't figured out the Makefile equivalent for the autoconf workflow,
but I'm digging into it now. Could someone please test this CMake change? I've
run it successfully on Windows/MSVC, but I don't have access to a CMake
environment on other platforms.
Original comment by kim.gras...@gmail.com
on 2 Feb 2013 at 2:50
Attachments:
Tested and works fine on Linux (CMake with Ninja). The .imp files were
correctly copied to the 'bin' directory in the build and install trees
respectively.
Original comment by zaheer.c...@gmail.com
on 4 Feb 2013 at 9:38
Thanks!
Waiting for Volodymyr's OK to commit.
Original comment by kim.gras...@gmail.com
on 5 Feb 2013 at 5:28
A few persons have expressed concerns installing .imp files together with
binaries. And I think copying .imp files next to executable file isn't the
right approach.
I'd like to make IWYU as easy to use as possible. External mappings is advanced
feature and applying this feature by default makes IWYU harder to use. All
these .imp files add complexity and make IWYU more fragile.
I think, keeping core mappings inside the executable will improve user
experience. By core mappings I mean libc, libstdc++, and gcc.symbols.imp.
Other [not so important] arguments in favor of keeping core mappings inside the
executable:
- we can use clang::Driver to detect STL version and apply version-dependent mappings. It is easier to do in IWYU itself;
- mappings won't break in case of symbolic links.
Kim, what do you think about it?
Original comment by vsap...@gmail.com
on 5 Feb 2013 at 11:13
I think you're right. External mappings have so far caused more noise than
added value.
I'm a bit concerned about portability -- what we currently see as 'core' is
just one set of mappings that happens to be almost ubiquitous in the Unix
world, but not on Windows. That said, Clang and IWYU does not really work well
enough to be usable with MSVC headers at this point.
There are a number of STL implementations out there, and I'm worried that their
mappings will conflict with libstdc++. The external mappings solve this nicely;
you'd create a new master mapping (like iwyu.gcc.imp) and ref in mappings that
make sense for your environment.
I think there would have to be a way to override 'core' mappings at least
symbol by symbol or #include by #include, but ideally whole named sets, e.g.
"STL" or "LIBC" to short-circuit IWYU's defaults in case they conflict with
your environment.
But I agree with your original concern, that the hard dependency on these
configuration files is unfortunate, and I'd like to back it out. I can think
about override policies later.
Let me see if I can come up with a patch to restore the core mappings, while
keeping support for external mappings.
Original comment by kim.gras...@gmail.com
on 6 Feb 2013 at 6:33
From limited personal experience with different libstdc++ versions (issue #88)
I can tell that differences between versions aren't severe. Public headers are
the same and missing private headers are just ignored. For example,
<tr1/functional> is present in all STL versions I've seen, but
<tr1_impl/functional_hash.h> is absent in some versions. And missing
<tr1_impl/functional_hash.h> is not a problem.
Original comment by vsap...@gmail.com
on 6 Feb 2013 at 9:52
Yes, I'm not so worried about differences between libstdc++ versions, I'm
guessing it's fairly stable at this point. I was thinking of alternative
implementations:
http://en.wikipedia.org/wiki/Standard_Template_Library#Implementations
Original comment by kim.gras...@gmail.com
on 6 Feb 2013 at 10:10
Attached is a patch that restores the hard-coded mappings for:
1) GNU libc and libstdc++ include mappings
2) GNU libc and libstdc++ symbol mappings
3) Hard-wired IWYU test include mappings
Google's third-party mappings are no longer added anywhere.
It also improves somewhat on the mapping management functions. It still allows
custom mappings being added with external mapping files.
Once this is committed, I'd like to:
0) Add the mapping fixes for tr1/functional from issue #88
1) Remove the concept of explicit mapping file search paths, and only use the
automatic addition of current mapping path for refs.
2) Pull iwyu_tests_include_map out into an external mapping file only used for
the tests that need those mappings.
3) Add a --no-default-mappings switch to start clean, so that people in non-GNU
environments can fall back to using custom external mappings without risk of
conflict with hard-coded default mappings (does this sound reasonable?)
Please review!
Original comment by kim.gras...@gmail.com
on 6 Feb 2013 at 9:07
Attachments:
Revised patch attached, this time with all mappings in a dedicated file,
iwyu_mappings.h. It seems nicer to me this way.
Original comment by kim.gras...@gmail.com
on 9 Feb 2013 at 10:04
Attachments:
Currently I have an error
iwyu_include_picker.h:73:6: error: ISO C++ forbids forward references to 'enum'
types
Let's discuss adopting C++11 separately and in this patch don't use enum
forward declaration.
Small indentation note - when long line wraps, it is aligned with some symbols
on the previous line like
for (foo
bar) // b is under f
or indented by 4 spaces [1].
Everything else, including plan of future actions, looks good.
[1]
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Function_
Declarations_and_Definitions#Function_Declarations_and_Definitions
Original comment by vsap...@gmail.com
on 10 Feb 2013 at 5:42
Oh, I didn't know forward-declaring enums wasn't allowed until C++11. MSVC
happily allowed it.
Unfortunately the separate header file hinges on the enum forward-declaration,
so I'll probably go back to the patch before.
I'll go over indentation again and make sure it's correct, thanks!
Original comment by kim.gras...@gmail.com
on 10 Feb 2013 at 10:00
New patch attached:
- Fix indentation
- Put all core mappings back into iwyu_include_picker.cc, since I couldn't
forward-declare enum
Let me know if this is good to go in, and I can send remaining patches on top
of that for review. Thanks!
Original comment by kim.gras...@gmail.com
on 12 Feb 2013 at 9:03
Attachments:
default_mappings-3.patch looks good to me.
Original comment by vsap...@gmail.com
on 14 Feb 2013 at 9:50
Thanks, committed in r445. I have a few more changes planned on top of this as
mentioned in comment #15, but I think r445 is enuogh to close this issue.
alexey.ivanes, could you build HEAD and see if IWYU starts as expected?
I think we should log the Qt problems as separate issues, I can take care of
that.
Original comment by kim.gras...@gmail.com
on 15 Feb 2013 at 6:09
> 0) Add the mapping fixes for tr1/functional from issue #88
Done.
> 1) Remove the concept of explicit mapping file search paths,
> and only use the automatic addition of current mapping path for refs.
Done.
> 2) Pull iwyu_tests_include_map out into an external mapping file only
> used for the tests that need those mappings.
Not done yet.
> 3) Add a --no-default-mappings switch to start clean, so that
> people in non-GNU environments can fall back to using custom
> external mappings without risk of conflict with hard-coded default
> mappings (does this sound reasonable?)
Attached patch for this -- I'd love for someone to look it through before I
commit. I'm especially interested in hearing any ideas for alternative
(shorter) switch names.
Thanks!
Original comment by kim.gras...@gmail.com
on 1 Mar 2013 at 9:22
Attachments:
> 2) Pull iwyu_tests_include_map out into an external mapping file only
> used for the tests that need those mappings.
r455.
Original comment by kim.gras...@gmail.com
on 3 Mar 2013 at 7:54
In iwyu_globals in comment is used -n flag, while it is actually -s flag.
I think it is a good practice to make constructor IncludePicker(bool) explicit.
Idea for switch name --reset-mappings. I don't think it's better than
--no-default-mappings, just an idea.
Otherwise looks good.
Original comment by vsap...@gmail.com
on 3 Mar 2013 at 9:16
Thanks for review!
> In iwyu_globals in comment is used -n flag, while it is actually -s flag.
This gets me every time. The actual flag is now -n.
> I think it is a good practice to make constructor IncludePicker(bool)
explicit.
Thanks, done.
> Idea for switch name --reset-mappings. I don't think it's
> better than --no-default-mappings, just an idea.
Maybe skip the "default" and just call it "--no_mappings"? It's not as clear,
but a little shorter, and you'd need to read the help string anyway to
understand what it's good for.
Original comment by kim.gras...@gmail.com
on 4 Mar 2013 at 6:05
Attachments:
Looks good. I am OK with both --no-default-mappings and --no-mappings.
Personally I prefer to sacrifice brevity in favor of correctness.
Original comment by vsap...@gmail.com
on 7 Mar 2013 at 9:07
Thank you -- r457.
I think that closes everything we can do around craziness with mappings. It
would be nice to hear from alexey.ivanes whether this fixes his specific issue
before we close the issue.
Original comment by kim.gras...@gmail.com
on 7 Mar 2013 at 3:44
It's been more than a year without feedback, I'm closing this as the gist of
the problem has been addressed.
Original comment by kim.gras...@gmail.com
on 6 Jun 2014 at 1:11
Original issue reported on code.google.com by
alexey.ivanes@gmail.com
on 31 Jan 2013 at 12:54Attachments: