Closed GoogleCodeExporter closed 9 years ago
Thanks for the patch! In general, it looks good to me.
We're looking to revamp iwyu_include_picker -- I was hoping this week, but it
will probably slip into next. The revamp will, as planned, get rid of the
distinction between C and C++ includes, to make it easier to add dynamic
private->public mappings. So it probably makes sense for you to wait for that
and then redo the patch with the new functionality. Hopefully it will have
fewer FIXMEs then.
In the meantime, it sounds like we'll need to get the clang patch in upstream
before we can apply the iwyu patch. Do you want to send it to
cfe-commits@cs.uiuc.edu and see what they think?
Original comment by csilv...@gmail.com
on 17 Feb 2011 at 10:14
That sounds good - I'm happy to rework the patch when the work on
iwyu_include_picker is done.
I've bounced the patch over to cfe-commits to get some feedback.
Original comment by paul.hol...@gmail.com
on 18 Feb 2011 at 2:33
I saw your post to cfe-commits but haven't seen any feedback yet. I'm not
familiar enough with clang development to know what a good next step might be...
btw, the revamp of iwyu-include-picker is in, so feel free to take another shot
at the patch.
Original comment by csilv...@gmail.com
on 23 Feb 2011 at 2:17
I think it's probably quite easy for patches like mine to be overlooked. I'll
ping the list about the patch on Friday (a week after I first submitted).
Failing that, my parent organisation has a couple of committers - I'll make
some inquiries to see if they'd be willing to sponsor the patch.
I saw the include-picker revamp. I'm re-working my patch, but have encountered
a bit of ugliness that I'm trying to resolve. Now that ConvertToQuotedInclude
is a non-member function, it no longer has access to the system_include_dirs_
state that I added to IncludePickers.
With a lot of plumbing, I can pass this through to ConvertToQuotedInclude in
the various places that it's called. But it seems like there's no reason to
keep this state with IncludePicker now.
My current plan is to add a new SearchPaths class (which encapsulates
system_include_dirs for now) and is constructed alongside include_picker in
InitGlobals(). ConvertToQuotedInclude could just access this directly, or could
be later be made a member function of SearchPaths. That seems like it'll be a
nicer fit to me. I'll put another patch together to get your thoughts.
Original comment by paul.hol...@gmail.com
on 23 Feb 2011 at 9:56
This is what I was thinking of. It still needs a little work to move
SearchPaths out of iwyu_include_picker.h/cc, but I was keen to get your
thoughts on the approach of adding some more global state like this.
Original comment by paul.hol...@gmail.com
on 23 Feb 2011 at 10:21
Attachments:
I like the basic idea, and have no problem with adding the global state. The
implementation is mostly good, but as you point out having SearchPaths in
iwyu_include_picker is a bit awkward.
What I would do is just keep the search-paths as a vector of strings, rather
than encapsulating them in a struct. GetSearchPaths then returns
vector<string>. And then in ConvertToQuotedInclude, just manually iterate over
the vector and compare prefixes. I don't think we need a separate method for
that (though you could add a FindPrefix to iwyu_stl_util.h, or maybe
iwyu_string_util.h, if you wanted to).
I think it may be a good idea to get the system dirs before case 2, not before
case 3. In case 2, we should only accept dirs with "/c++/" in them if they
also start with a system dir. This will protect against user code that uses a
/c++/ directory, being marked as a system dir.
Original comment by csilv...@gmail.com
on 24 Feb 2011 at 7:14
That all sounds good. I'm away for a week to attend GDC, but when I'm back I'll
have a look at making those changes.
I pinged cfe-commits about the patch to expose HeaderSearch::SearchDirs so
hopefully there'll be some progress on that during the week.
Original comment by paul.hol...@gmail.com
on 25 Feb 2011 at 4:57
My patch to Clang was accepted at r127122 here:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20110228/039708.html.
Hurrah!
I've updated my search dir patch based on your feedback. I've pulled the code
for what was Case 2/3 out into a NormalizeSystemPath function as it seemed to
make things more readable.
A couple of things:
1) In the previous patches I submitted, I provided fallback code which stripped
past '/include/'. Dropping this fallback allowed me to simplify things a bit
more, but it's hard for me to test this on other systems.
2) I wonder if the explicit handling for c++ dirs/'asm-' is actually needed
now? Now that we're enumerating the search paths as seen by Clang, we should
get this all for free and NormaliseSystemPath can be removed entirely? Again,
this is hard for me to test on other systems.
Original comment by paul.hol...@gmail.com
on 8 Mar 2011 at 4:14
Attachments:
Yay!
The patch looks great. I'll look to apply it today. But maybe we can figure
out some of your question-items first (or they could always be a second patch).
} I wonder if the explicit handling for c++ dirs/'asm-' is actually needed now?
What does GlobalSearchPaths() return for you? Can you just append the full
list to this issue? I can compare it to mine and see what seems to be standard
and what seems to vary. My guess is that we can drop NormalizeSystemPath now.
} I provided fallback code which stripped past '/include/'. Dropping this
fallback
I think dropping the fallback should be fine, for the same reason.
Original comment by csilv...@gmail.com
on 8 Mar 2011 at 7:19
OK, I looked at your two issues, and we can't remove these functions as-is.
We could drop NormalizeSystemPath() if we sorted the global search path by
length, to guarantee getting the longest prefix match. Right now, the global
search path has both
/usr/include/c++/4.4
and
/usr/include/c++/4.4/x86_64-linux-gnu
It would be bad if we matched the former when we could have matched the latter.
The asm- mapping should be done in AddDirectInclude(), where we already do
mappings of this sort.
I've added a TODO for both of these and committed your change (with some minor
formatting fixes) as r58. Feel free to tackle those two TODOs if you'd like!
(I'll leave the bug open until they're done.)
Original comment by csilv...@gmail.com
on 8 Mar 2011 at 10:06
Ah - well spotted. For reference, I see this:
<user-search-paths>
C:/dev/llvm_build/bin/Debug/../lib/clang/2.9/include
c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\include
C:\Program Files (x86)\Microsoft SDKs\Windows\v7.0A\\include
<user-search-paths> are those passed to clang/iwyu via -I. The remaining three
paths come from clang/lib/Frontend/InitHeaderSearch.cpp (I actually build our
code with -nostdinc and -nostdinc++, which suppresses this, so that I can
compile against different system headers)
I'm happy to look at tackling those TODOs. I'm also keen to make StripLeft a
bit more robust when dealing with non-canonical paths (to handle Win32 case
insensitivity, embedded '../', '\\', and forward vs backwards path separators -
all of which can be seen in my example above!)
Thanks for committing the patch!
Original comment by paul.hol...@gmail.com
on 8 Mar 2011 at 10:33
Great! Let me know if you have a new patch for the TODOs.
StripLeft is a string routine and should not be knowledgeable about paths. But
it would be great if you could add a path routine that was better than
StripLeft to use here. See also issue 16, which is related to this I think,
and also maybe issue 5.
Original comment by csilv...@gmail.com
on 8 Mar 2011 at 10:39
Hi,
I've been looking at fixing these TODOs. For the asm- mapping I have this code
following the /internal/ logic in AddDirectInclude:
if (StartsWith(include_name_as_typed, "<asm-")) {
AddMapping(include_name_as_typed, quoted_includer);
MarkIncludeAsPrivate(include_name_as_typed);
if (quoted_includer.find("<asm-") != string::npos)
MarkIncludeAsPrivate(quoted_includer);
}
It works for my simple test cases and correctly suggests "<asm/foo.h>" rather
than "<asm-ARCH/foo.h>". However I'm unsure that I understand the logic fully.
As I understand, when AddDirectInclude sees a 'private' #include (e.g.
/public/foo.h which includes /internal/foo.h) this adds a mapping from
/internal/foo.h -> /public/foo.h. I guess this is being somewhat trustworthy
and relying on users of the library not to reference /internal/foo.h directly?
Here's a concrete example:
<system path>/asm/foo.h:
#include <asm-hello/foo.h>
<system path>/asm-hello/foo.h:
#define SOME_SYMBOL 42
user.h:
#include <asm-hello/foo.h> // A bit naughty, should be <asm/foo.h>
user.cpp:
#include "user.h"
int main()
{
return SOME_SYMBOL;
}
In this case I'd expect IWYU to tell me to remove the reference to
<asm-hello/foo.h> in user.h and replace it with <asm/foo.h> in user.cpp, but it
reports no errors.
I suspect this might be a separate issue/non-issue. That aside, does it look
like I'm on the right track with my changes to AddDirectInclude?
Cheers,
Paul
Original comment by paul.hol...@gmail.com
on 11 Mar 2011 at 12:20
Yeah, you're slightly off -- your code does correctly say that asm-hello/foo.h
should map to asm/foo.h, but you're saying that asm-hello/foo.h can also map to
user.h. So iwyu isn't complaining because it says 'user.cpp needs SOME_SYMBOL
from asm-hello/foo.h, but I can map that need to user.h, and look, user.cpp is
#including that, so I'm fine.'
You probably want code like this:
// If we see that <asm/foo.h> is #including <asm-xxx/foo.h>, then we assume
// asm-xxx/foo.h is a private include, and all uses of that should be mapped
// to asm/foo.h.
if (StartsWith(include_name_as_typed, "<asm-") &&
StuffAfter(include_name_as_typed, "<asm-[^/]*/") == StuffAfter(quoted_includer, "<asm/")) {
AddMapping(include_name_as_typed, quoted_includer);
MarkIncludeAsPrivate(include_name_as_typed);
}
Obviously the "StuffAfter" part is pseudo-code. :-) I'm not actually
suggesting you implement StuffAfter, but you can use StripPast and what-not to
achieve the same effect.
Does this make sense?
Original comment by csilv...@gmail.com
on 11 Mar 2011 at 12:38
The code I mentioned in the last comment got added in at some point, I think.
I believe this bug is fixed. Feel free to reopen if there are still
outstanding issues.
Original comment by csilv...@gmail.com
on 25 Oct 2011 at 12:11
Original issue reported on code.google.com by
paul.hol...@gmail.com
on 17 Feb 2011 at 9:12Attachments: