Open GoogleCodeExporter opened 9 years ago
Thanks for the report. Am glad to take a patch that fixes it! Or I guess we
could just document it...
Original comment by csilv...@gmail.com
on 7 Feb 2011 at 7:42
I'm not sure if this is exactly the same issue, but I'm very interested in
getting IWYU running over WebKit.
Currently, the biggest blocker is that WebKit doesn't fully specify the path
for includes. So IWYU comes up with a lot of what I'd consider to be false
positives like:
... should add these lines:
#include "../../WebCore/dom/Document.h"
... should remove these lines:
#include "Document.h"
If you can point me in the right direction, I'd be happy to work on a path to
at least make it an option to ignore these types of changes.
Original comment by tonyg@chromium.org
on 4 Mar 2011 at 11:15
Hmm, I'm not sure I understand what's going on here. Is the iwyu
recommendation coming from a file that's in the WebCore/dom directory? So
../../WebCore/dom/Document.h is the same as Document.h?
Do you know where these filenames are coming from? I think ../../whatever must
be coming from GetFilePath(decl), which just calls file->getName() on decl's
FileEntry. Maybe you can run in gdb, or put in print statements, to verify
that getName() is returning the path with all these ..'s in it.
If so, we need to figure out if that's the right thing to be doing or not.
It may be that you just need to augment CanonicalizeFilePath in
iwyu_path_util.h. There's even a comment saying we may want to collapse ..'s.
CanonicalizeFilePath would need to know what directory we're canonicalizing
from, which may be difficult to thread through to that routine. But once you
have that info, you could get rid of ../xx/ when possible.
Original comment by csilv...@gmail.com
on 4 Mar 2011 at 11:40
../../WebCore/dom/Document.h is a correct path, but the style in WebKit is to
omit the paths. I understand that is probably inefficient since there is a lot
more searching going on, but it would be hard to change at this point.
All the include paths are included via -I, for example:
-I../../WebCore/dom
Original comment by tonyg@chromium.org
on 5 Mar 2011 at 12:17
I see, in that case the best place to do things is probably iwyu_output.cc.
I'm not exactly sure how to architect it. But basically you want to look at
all the include paths (you'll have to get this from clang, which maybe doesn't
have to code to provide it yet), and then see if anything from the set of
'desired includes' is actually the same as something in 'direct includes', when
the search paths are taken into account. If so, then you can rename the header
file for the desired include. Or something like that.
Original comment by csilv...@gmail.com
on 5 Mar 2011 at 2:01
(See also issue 12, regarding get the list of system include dirs)
Original comment by csilv...@gmail.com
on 5 Mar 2011 at 2:04
Looks like the patch in issue 12 will go a long way towards solving this (if
not completely). I'll hold off until that lands and then use it as a starting
point if there is still more to be done here.
Original comment by tonyg@google.com
on 10 Mar 2011 at 2:19
Most of issue 12 has already been committed, so you may want to try again from
svn-root. I'm not exactly sure how much it's helped.
Original comment by csilv...@gmail.com
on 10 Mar 2011 at 7:02
The attached patch causes iwyu to produce correct output for the WebKit
project. However, I haven't tested it anywhere else, so I'm not sure if it is
generally appropriate.
Note that this doesn't affect issue 16.
Original comment by tonyg@chromium.org
on 14 Mar 2011 at 9:32
Attachments:
Nice idea -- now that we actually know all the system directories, it's safe to
check for them first. This also makes clearer the unexpected third case, which
is an absolute include that is not a system directory. I don't know how to
handle those.
The problem with this patch -- which isn't your fault -- is that 'system'
directory is a misnomer now, since the list includes directories added via -I,
which are not system directories and should use "", not <>.
In practice, that means if you do -Ifoo, and then do #include "bar.h" to get
foo/bar.h, with this patch iwyu will want to change it to #include <bar.h>. I
think. I'd expect this to have triggered for you, so I may be wrong. Is this
not what you saw?
I think we'll need to augment the vector of system dirs to distinguish between
system and non-system dirs. It will need to look at whether the dir was added
via -I or -isystem (or, perhaps, after -I-). gcc-compatible include-flag
parsing is annoying, so hopefully clang/llvm does all the work for us, and we
just need to expose it.
Original comment by csilv...@gmail.com
on 14 Mar 2011 at 10:53
As part of a patch to fix this, can you add a test to the tests/ directory?
This will require a minor change to the run_iwyu_tests.py framework. I think
what you'll want to do is create a subdir of tests, maybe
tests/a/b/something.h, and have a few .h files that #include something.h (and
use a symbol from it) in different ways: as "a/b/something.h", "b/something.h",
"something.h", where we compile the relevant .cc file with -Ia -Ia/b -Ia/b/
-Ia/b/../b. Ideally we'd also test with -I/<path_to_iwyu>/a/b, which should be
possible inside run_iwyu_tests.py.
The change required to the test framework is to make it possible to pass not
only iwyu_flags to iwyu_test_util.py, but also clang flags. Then
iwyu_test_util.py will need to add in these flags when compiling, just like it
adds in iwyu flags now.
Original comment by csilv...@gmail.com
on 14 Mar 2011 at 11:00
Clang's HeaderSearch does have a SystemDirIdx, which should allow us to handle
system/user dirs differently (I was careful when submitting my patch to Clang
to expose both search_dir_begin/end and system_dir_begin/end, in case we did
end up needing it). I'm assuming that means it handles -isystem etc, but I will
check this now.
Original comment by paul.hol...@gmail.com
on 14 Mar 2011 at 11:38
Ok, it seems that Clang supports -iquote and -isystem, but not -I-. -Ifoo adds
foo as a system dir (and so I think Tony would need to use
-iquote../../WebCore/dom, not -I../../WebCore/dom)
Enumerating header_search->search_dir_begin()..end() yields 'user' dirs first,
then system dirs (when it == header_search->system_dir_begin()). Bodgy example:
static vector<string>* MakeSearchPaths(clang::HeaderSearch* header_search) {
bool system = false;
vector<string>* search_paths = new vector<string>;
for (clang::HeaderSearch::search_dir_iterator
it = header_search->search_dir_begin();
it != header_search->search_dir_end(); ++it) {
const DirectoryEntry * entry = it->getDir();
if( it == header_search->system_dir_begin() )
system = true;
if (entry != NULL) {
printf( "%s %s\n", system ? "system" : "search", it->getName() );
search_paths->push_back(entry->getName());
}
}
...
(enumerating user and system dirs separately is a bit more clumsy than I'd have
liked. Retrospectively I should have added user_dir_begin/end() too.)
Original comment by paul.hol...@gmail.com
on 15 Mar 2011 at 12:21
issue 16 has a patch for this issue
Original comment by thakis@chromium.org
on 7 May 2011 at 12:01
Have checked the original test case - the problem is still present. Also there
is a problem with relative path, when files are
// Foo.h
class Foo {
Foo();
};
// Foo.cc
#include "Foo.h"
Foo::Foo() {}
when we run IWYU receive the following output
$ /path/to/include-what-you-use tests/Foo.cc
(tests/Foo.h has correct #includes/fwd-decls)
tests/Foo.cc should add these lines:
#include "tests/Foo.h"
tests/Foo.cc should remove these lines:
The full include-list for tests/Foo.cc:
#include "tests/Foo.h"
#include "Foo.h" // for Foo
---
Seems that patch provided by Tony at
http://code.google.com/p/include-what-you-use/issues/detail?id=16#c19 resolves
this issue, at least partially.
Original comment by vsap...@gmail.com
on 1 Oct 2011 at 2:38
Sorry, forgot to mention that it is for iwyu built from r320.
Original comment by vsap...@gmail.com
on 1 Oct 2011 at 3:03
Attached a patch which resolves this issue. In short: use for IwyuFileInfo
quoted_file as it was included, not its file path. Thus associated header use
is ignored when compared with actual includes.
But I have a few questions:
* Should I update test runner to test invoking iwyu with absolute path,
with/without "-I ."? I've checked that manually, but probably an automated test
will be more appropriate.
* My patch affects IwyuFileInfo::AssociatedQuotedIncludes() and it seems to be
correct, but I don't have test for AssociatedQuotedIncludes(). Should I create
one?
Original comment by vsap...@gmail.com
on 6 Oct 2011 at 6:06
Attachments:
Thanks for the patch! I think there are several places where we use the
filename where we'd be better off using a quoted-include.
One thing I notice about your patch is it only affects existing includes -- if
we were to add a new include, we'd still suggest the full path-name, rather
than using the -I flags we have. Is that desirable?
The example you have in comment 15 is an interesting one -- it's specifically
about the associated-includes (as you point out), which we always try to add
even if they're not strictly necessary: We have foo.cc #include foo.h even if
it's not 'using' anything from foo.h. Maybe the patch you have is only meant
to address that issue, and not the broader one of seeing the same filename
through two different paths due to -I?
I'd love to see one patch that fixes all the problems we've been having with -I.
What happens when you apply the patch from
http://code.google.com/p/include-what-you-use/issues/detail?id=16#c19
Does that also fix this problem? If so, it may be better to just try to get
that into a shape we can submit it.
Original comment by csilv...@gmail.com
on 6 Oct 2011 at 9:50
[deleted comment]
[deleted comment]
[deleted comment]
Patch from
http://code.google.com/p/include-what-you-use/issues/detail?id=16#c19
doesn't fix associated header include.
ConvertToQuotedInclude("tests/header_near.h") == ""tests/header_near.h"" // all
paths from GlobalHeaderSearchPaths() are rejected and we use Case 2 return "\""
+ path + "\"";
At first I wanted to rework ConvertToQuotedInclude so it returns result like
typed. But then I've created a few test cases and have seen that iwyu fails
only for associated headers. So I've decided to fix associated headers only.
But I have an idea how we can obtain better results in ConvertToQuotedInclude -
to add main compilation unit directory as the last header search path. Thus
GlobalHeaderSearchPaths() will look roughly like
/usr/include/c++/4.0.0/i686-apple-darwin10 (system)
. (user)
tests (user)
In this case if we provide -I . ConvertToQuotedInclude("tests/indirect.h") ==
""tests/indirect.h""
if don't provide -I . ConvertToQuotedInclude("tests/indirect.h") ==
""indirect.h""
Original comment by vsap...@gmail.com
on 7 Oct 2011 at 12:32
} At first I wanted to rework ConvertToQuotedInclude so it returns result like
typed.
} But then I've created a few test cases and have seen that iwyu fails only for
} associated headers. So I've decided to fix associated headers only.
Hmm, so maybe we are good about using the name-as-typed in most places. We
just miss it for associated headers.
Looking at the patch, I find it a bit suboptimal that we set the quoted-name
based on the filename, and then override based on the name-as-typed. I think
it may be cleaner if we just pass in the quoted-name as an argument, and not
have the set_quoted_name method you added (I'm going from memory, the names may
not be exactly right). What do you think?
craig
Original comment by csilv...@gmail.com
on 7 Oct 2011 at 12:42
Also, could you please sign the CLA if you haven't already?
http://code.google.com/legal/individual-cla-v1.0.html
That way I can accept the patch.
Original comment by csilv...@gmail.com
on 7 Oct 2011 at 12:43
} Looking at the patch, I find it a bit suboptimal that we set the quoted-name
based on the filename, and then override based on the name-as-typed.
Agree, setting quoted_file_ once is safer than changing it. Updated patch is
attached. Have signed CLA.
Original comment by vsap...@gmail.com
on 7 Oct 2011 at 4:15
Attachments:
And here is a test case for adding new include (in case somebody else want to
fix this issue):
// tests/header_near_new_include.h
#include "indirect.h"
// tests/header_near_new_include.cc
#include "header_near_new_include.h"
void foo() {
IndirectClass ic;
}
$ include-what-you-use -Xiwyu --verbose=3
/Users/vsapsai/Projects/OpenSource/llvm/llvm/tools/clang/tools/include-what-you-
use/tests/header_near_new_include.cc
<skipped>
/Users/vsapsai/Projects/OpenSource/llvm/llvm/tools/clang/tools/include-what-you-
use/tests/header_near_new_include.cc should add these lines:
#include
"/Users/vsapsai/Projects/OpenSource/llvm/llvm/tools/clang/tools/include-what-you
-use/tests/indirect.h" // for IndirectClass
Original comment by vsap...@gmail.com
on 7 Oct 2011 at 4:22
OK, the patch in comment 25 is checked into svn!
I'm keeping this bug open because I'm not yet sure that all cases it refers to
are fixed. If they are, someone shout and I'll close the bug.
Original comment by csilv...@gmail.com
on 7 Oct 2011 at 9:51
I am developing a patch to fix this issue and have the following question. Is
it expected for:
// tests/new_header_path_provided.h
#include "indirect.h"
// tests/new_header_path_provided.cc
#include "tests/new_header_path_provided.h"
void foo() {
IndirectClass ic;
}
$ include-what-you-use -Xiwyu --verbose=3 -I tests/..
tests/new_header_path_provided.cc
to offer to add #include "indirect.h", though with path "tests/.." we can
access this file via #include "tests/indirect.h"?
And for different include search paths we receive following recommendations:
-I tests/.. #include "indirect.h"
-I . #include "tests/indirect.h"
<no include search path> #include "indirect.h"
Original comment by vsap...@gmail.com
on 20 Nov 2011 at 8:47
A good question. It's a hard problem to know what to recommend when the same
file is available via multiple search paths.
} And for different include search paths we receive following recommendations:
} -I tests/.. #include "indirect.h"
} -I . #include "tests/indirect.h"
} <no include search path> #include "indirect.h"
This is reasonable. For "-I .", I could see suggesting either tests/indirect.h
or just indirect.h.
The 'right' thing to do is to figure out which of the possible legal
include-paths others parts of the codebase are using. If there's other code
that uses "tests/indirect.h", then probably that's what iwyu should suggest.
This shouldn't even be that hard. If iwyu is suggesting you add foo.h, then
that means that foo.h should have been seen somewhere in the transitive
#includes. We should just copy the spelling used there. In the rare cases the
#include *isn't* found elsewhere in the translation unit, we can just pick a
spelling arbitrarily.
Original comment by csilv...@gmail.com
on 21 Nov 2011 at 7:53
I hope I've fixed this issue by adding main compilation unit directory to
header search paths. This directory is used when there are no appropriate
header search paths. Patch is attached.
I've extended testing script to test the same file with different flags and
with relative/absolute path (based on patch at
http://code.google.com/p/include-what-you-use/issues/detail?id=16#c19 and
related discussion). Don't know if it is an appropriate solution and it is
pretty messy now, for example RunOneTest can run more than 1 test. Can you look
at it and point out a direction for improvement?
Original comment by vsap...@gmail.com
on 6 Dec 2011 at 11:58
Attachments:
I think it is indeed the right approach. I don't know if it resolves the
entire issue, but it's clearly a bug now that we're not considering the
path-that-the-.cc-file-is-in in the search path. Then again, I'm a little
surprised that path isn't there by default. I wonder if we're missing
something that would get clang to give it to us.
btw, according to
http://gcc.gnu.org/onlinedocs/cpp/Search-Path.html
and other sources, compilers look in the directory-of-.cc-file first, not last.
The code in iwyu_globals and iwyu_path_util look good. Only concern is a style
nit: we use underscore_names for vars rather than camelCase.
I'd rather not see such drastic changes to the test suite. I'm happy adding
clang_flags as an analog to iwyu_flags. But all the other stuff you do to
support testing one file with an absolute pathname (as I understand it), I
think is better done in some other way. We could figure out some way to run
the test not as part of this test framework, perhaps. Or perhaps you can
modify RegisterFileForTesting to special-case the one file we want to test with
an absolute filename, and have it register the full filename rather than the
relative name. That has a small chance of Just Working, and a larger chance of
working with relatively minor modifications.
Original comment by csilv...@gmail.com
on 7 Dec 2011 at 12:43
I've updated the patch according to your feedback, please find attached.
Path-that-the-.cc-file-is-in is the last in the header search path because with
"-I ." and main file directory "tests" I want "tests/indirect.h" to match as
.+tests/indirect.h, not as tests/+indirect.h.
As for test suite changes, I've left only clang_flags.
Original comment by vsap...@gmail.com
on 7 Dec 2011 at 10:50
Attachments:
OK, looks good. I want to pass it by one more set of eyes than my own, but
will get it submitted after that.
Original comment by csilv...@gmail.com
on 7 Dec 2011 at 11:12
Just wanted to let you know I haven't forgotten about this! The person I
wanted to look at it is busy until next week, but I'll check it in once he has.
Original comment by csilv...@gmail.com
on 9 Dec 2011 at 10:14
I'm glad I waited for comments! The other reviewer found one major issue and
several minor ones.
The major issue is that the logic of just adding the directory of the current
.cc file is wrong. It is meant to be the current file being parsed. So if we
have
foo/test.cc: #include "subdir/a.h"
foo/subdir/a.h: #include "b.h"
foo/subdir/b.h: whatever
This should work. But the way the patch works now, we won't properly handle
the "b.h" include.
I don't know how big a problem this is in practice. I do suspect the current
way you have of handling this code will not easily extend to handle the case
above, if we ever need to, so I think it makes sense to redo it a different
way. Perhaps the right place to look is the place(s) where we examine the path
-- in each of those, add in another function that is like
SearchPaths(current_file) that adds in the appropriate directory and returns
the resulting search paths.
Here are the minor comments he had. The line numbers may not exactly match up
with what you have, but there should be enough context to figure out what he's
talking about...
========================================================================
https://mondrian.corp.google.com/file/26046161///depot/google3/devtools/maintena
nce/include_what_you_use/iwyu_path_util.cc?a=1
File
//depot/google3/devtools/maintenance/include_what_you_use/iwyu_path_util.cc
(snapshot 1)
------------------------------------
Line 164: // If *path starts with prefix_path, removes the prefix and path
Probably should be imperative: "removes" instead of "remove".
------------------------------------
Line 165: // separator from *path and returns true. Otherwise, return false.
"return true", not "returns true". Alternatively, "returns false" and don't make
the above suggested change.
------------------------------------
Line 166: static bool StripPath(string *path, const string& prefix_path) {
Style nit: "string* path", not "string *path".
Also, the order of arguments is not iwyu style. Although since path is
input-output it may be ok.
------------------------------------
Line 167: bool has_stripped_prefix = StripLeft(path, prefix_path);
I'm not fond of this implementation -- it will strip the prefix if it's not a
path component.
Perhaps the way to do this is to eliminate this routine entirely and have the
caller call StripLeft with a path ending in "/".
========================================================================
https://mondrian.corp.google.com/file/26046161///depot/google3/devtools/maintena
nce/include_what_you_use/iwyu_path_util.cc?a=0
File //depot/google3/devtools/maintenance/include_what_you_use/iwyu_path_util.cc
------------------------------------
Line 171: if (StripLeft(&path, it->path)) {
This looks to be an existing bug -- I think a path separator character should be
appended to it->path if necessary before stripping.
========================================================================
https://mondrian.corp.google.com/file/26046161///depot/google3/devtools/maintena
nce/include_what_you_use/iwyu_test_util.py?a=1
File
//depot/google3/devtools/maintenance/include_what_you_use/iwyu_test_util.py
(snapshot 1)
------------------------------------
Line 380: clang_flags = clang_flags or ['-I .'] # Reasonable default.
I'm not sure this really is a "reasonable default". What's the motivation for
that? That being said, I see this is a rewording of existing practice.
========================================================================
Original comment by csilv...@gmail.com
on 12 Dec 2011 at 7:24
Thanks for the feedback.
What exactly do you mean by
> we won't properly handle the "b.h" include.
? I've tried the provided files structure where "foo/test.cc" requires
"foo/subdir/b.h" and received
$ include-what-you-use -Xiwyu --verbose=3 tests/test.cc
<snip>
The full include-list for tests/test.cc:
#include "subdir/b.h" // for IndirectClass
This result seems correct to me. I'll look for more errors but want to
understand what do you mean.
And about code review
>Line 171: if (StripLeft(&path, it->path)) {
I guess it should be "if (StripPath(&path, it->path)) {", am I correct?
Original comment by vsap...@gmail.com
on 12 Dec 2011 at 11:01
} "foo/test.cc" requires "foo/subdir/b.h"
Yes, that part is fine. The problem is when foo/subdir/b.h includes its own
file in turn.
Original comment by csilv...@gmail.com
on 12 Dec 2011 at 11:12
Still don't get what you are talking about. Do you mean there is a problem when
foo/subdir/b.h includes other files and we invoke iwyu with
--check_also=foo/subdir/b.h?
If it's not what you mean, can you please provide a small test case and
expected result?
Original comment by vsap...@gmail.com
on 13 Dec 2011 at 8:32
It's probably me who doesn't understand. Can you go back to first principles
and explain what problem your patch addresses? I thought it was addressing the
fact that the search path we used in iwyu doesn't match what clang uses
internally. But maybe it's addressing something else.
Original comment by csilv...@gmail.com
on 14 Dec 2011 at 1:58
I am trying to fix using absolute paths when
- iwyu is invoked with absolute files' paths and;
- included files are discovered because they are in the same directory as .cc
file, not via provided header search paths.
To fix this problem I've added checking if included file can be found in
directory-that-the-.cc-file-is-in.
It is good to have search path in iwyu identical to clang search path. But I
don't know how it will affect file names in suggested include directives
because we want them to be nice and short while clang doesn't care about it.
Original comment by vsap...@gmail.com
on 15 Dec 2011 at 12:16
OK, but your fix has the side effect that it adds the directory-of-the-.cc file
as an iwyu search path for everyone, including .h files that we #include that
may be in totally different directories. If we have --check_also set, as you
say, this could give totally wrong #include suggestions.
I think the right thing to do is to get rid of iwyu's concept of the search
path entirely, and to instead reuse the spelling that iwyu already sees in its
analysis. A way to think of this is that iwyu never inserts totally new
#includes, it just copies an #include from one file to another. When we copy
the #include in that way, we should preserve the spelling.
The only hard part to deal with there is when the #include is depending on
'same file as includer' to be found. In that case, we will need to rewrite the
#include as we move it. So knowing the clang search path *will* be necessary
to figure out if that case applies.
I think I'd like to see this principled fix rather than trying to hack on to
the existing clang-search-path code, which I think is fundamentally not what we
want to be trying to do here.
(Note: if we do this we get unix vs. windows paths for free!)
Original comment by csilv...@gmail.com
on 15 Dec 2011 at 12:41
OK, I agree with you, will try to write a better fix.
Original comment by vsap...@gmail.com
on 16 Dec 2011 at 8:53
Any word on this?
Original comment by plaztiks...@gmail.com
on 16 Jul 2012 at 9:02
There is no much progress on this issue. I plan to resolve this issue after
fixing failing tests.
Original comment by vsap...@gmail.com
on 17 Jul 2012 at 8:35
Original issue reported on code.google.com by
jason.ha...@gmail.com
on 7 Feb 2011 at 7:24