LukasScheucher / include-what-you-use

Automatically exported from code.google.com/p/include-what-you-use
Other
0 stars 0 forks source link

Absolute for relative path replacement #5

Open GoogleCodeExporter opened 8 years ago

GoogleCodeExporter commented 8 years ago
When the compile line refers to a source file by its absolute name iwyu 
suggests that the includes be changed to absolute:

jason$ cat Foo.h
#ifndef _FOO_H_
#define _FOO_H_

class Foo {
  Foo();
};

#endif

jason$ cat Foo.cpp
#include "Foo.h"

Foo::Foo() {}

jason$ include-what-you-use /Users/jason/Desktop/iwyu/Foo.cpp

/Users/jason/Desktop/iwyu/Foo.cpp should add these lines:
#include </Users/jason/Desktop/iwyu/Foo.h>  // for Foo

/Users/jason/Desktop/iwyu/Foo.cpp should remove these lines:
- #include "Foo.h"  // lines 1-1

The full include-list for /Users/jason/Desktop/iwyu/Foo.cpp:
#include </Users/jason/Desktop/iwyu/Foo.h>  // for Foo

---

However when referring to the source file by relative name the output is as 
expected:

jason$ ~/Sources/llvm/build/bin/include-what-you-use Foo.cpp
(Foo.cpp has correct #includes/fwd-decls)

Original issue reported on code.google.com by jason.ha...@gmail.com on 7 Feb 2011 at 7:24

GoogleCodeExporter commented 8 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

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
../../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

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
(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

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
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:

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
issue 16 has a patch for this issue

Original comment by thakis@chromium.org on 7 May 2011 at 12:01

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
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:

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
[deleted comment]
GoogleCodeExporter commented 8 years ago
[deleted comment]
GoogleCodeExporter commented 8 years ago
[deleted comment]
GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
} 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

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
} 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:

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
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:

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
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:

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
} "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

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
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

GoogleCodeExporter commented 8 years ago
Any word on this?

Original comment by plaztiks...@gmail.com on 16 Jul 2012 at 9:02

GoogleCodeExporter commented 8 years ago
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