Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Analyze headers #16

Open
vsapsai opened this issue Jun 15, 2015 · 46 comments
Open

Analyze headers #16

vsapsai opened this issue Jun 15, 2015 · 46 comments

Comments

@vsapsai
Copy link
Contributor

@vsapsai vsapsai commented Jun 15, 2015

Originally reported on Google Code with ID 16

IWYU seems to only analyze .cc/.cpp files, however the biggest wins in terms of compile
time are found in the headers. I'm curious what it would take to be able to analyze
header files as well.

Reported by tonyg@chromium.org on 2011-03-05 00:07:47

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

iwyu analyzes .h files.  What is making you think it doesn't?

Reported by csilvers on 2011-03-05 00:16:32

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

After running over the WebKit project, I haven't found a single suggestion for a .h
file yet (maybe I'm missing them). Nico Weber reported a similar experience for Chromium.

Reported by tonyg@chromium.org on 2011-03-05 00:19:53

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

We only report for .h files with associated .cc files.  I think issue 5 may be related
here -- our IsAssociatedHeaderFile() check isn't firing because of the path issues.

We have a --check_also flag that one can use to specify .h files explicitly, but it
probably matches against FileEntry::getFileName() and thus would have the same problems
we see in issue 5.  Also, there's no way to actually specify check_also in opensource
iwyu yet.

I don't have a source tree with these problems, so it would be great if you could dig
into iwyu a bit and see why the .h files aren't getting picked up to report on!

Reported by csilvers on 2011-03-05 00:27:13

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

You were right, IwyuPreprocessorInfo::BelongsToMainCompilationUnit() is failing comparisons
like:

"../ui/base/animation/animation"
vs.
"/Volumes/work/chromium/src/app/../ui/base/animation/animation"

I'll put together a patch.

Reported by tonyg@chromium.org on 2011-03-14 22:32:18

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

Great.  I take it you're going to do something that properly collapses ../xxx?

When we have that, we need to remember to apply it to MakeSystemDirs (in iwyu_globals.cc).
 Right now, MakeSystemDirs() is returning entries for me like
   /home/csilvers/devel/llvm/Debug+Asserts/bin/../lib/clang/3.0/include
which I think will never be matched currently.

This is mostly a note so we don't forget.

Reported by csilvers on 2011-03-14 22:55:48

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

This patch takes care of reporting problems in header files for WebKit and Chromium.
Based on your comment in issue 5, I assume it needs tests.

I'm running on a mac and currently all tests fail because no output is printed. I'll
look into what is up with that then add tests for both patches.

Reported by tonyg@chromium.org on 2011-03-14 23:22:06


- _Attachment: [iwyu_issue_16.patch](https://storage.googleapis.com/google-code-attachments/include-what-you-use/issue-16/comment-6/iwyu_issue_16.patch)_
@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

Hmm, stripping off the cwd is interesting.  Should that perhaps be made part of CanonicalizeFilePath?
 We can say that we try to turn absolute paths into relative paths (relative to cwd)
if possible, as part of canonicalization.

And yes, tests would be great!  My head is starting to hurt thinking about the ways
all these paths interact.

Reported by csilvers on 2011-03-15 00:00:29

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

The tests all run for me now (after the fix in issue 21). However, in a clean client,
the badinc test fails. I've opened issue 22 to track.

Reported by tonyg@chromium.org on 2011-03-15 18:09:32

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

Moving the logic to CanonicalizeFilePath() was a good idea. Here's an updated patch
that doesn't cause any new regressions in the existing tests.

However, the right place to test this seems to be in more_tests/ rather than tests/,
but the wiki page says there is no framework for running those tests yet. Is there
an easy way I can run them? Or perhaps you have an idea of how to test this in tests/?
If not, I'll pick this patch up again once more_tests/ works.

Reported by tonyg@chromium.org on 2011-03-15 21:07:10


- _Attachment: [iwyu_issue16_2.patch](https://storage.googleapis.com/google-code-attachments/include-what-you-use/issue-16/comment-9/iwyu_issue16_2.patch)_
@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

Patch looks reasonable.  Can you update the comment to mention that we convert absolute
to relative paths when possible?

I think you should definitely be able to test this in tests/, we just need to figure
out the precise details. :-)  I don't know exactly how you were getting the problem
you were, but I'm guessing it's because you had -I directives with an absolute path
in them?

If so, you can make a new test in tests that does the same thing.  Maybe a simple .cc
file that #includes "direct.h" and uses Indirect, but iwyu is called with -I `pwd`
instead of -I . -- actually, just putting -I `pwd` in front of -I . should work ok.
 Or whatever it is you need to do to be able to replicate the problem you were seeing
(before your patch).

This will require some minor changes to the run_iwyu_tests framework so you can pass
iwyu flags to iwyu_test_util from run_iwyu_tests.py.  And you'll need to modify run_iwyu_tests.py
to use that new facility in your test, just like we do now with the clang-flags map.


Reported by csilvers on 2011-03-15 21:48:18

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

At r130 iwyu still not analyse headers with *.hpp extension. Is this patch applied to
trunk yet? My code base is mainly template in header, and iwyu seems to suggest me
#include already used by the header and therefore gives suggestion of extra header
in *.cpp. Thanks

Reported by wing1127aishi on 2011-04-15 18:50:09

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

No, I'm still waiting on the latest version with tests -- Tony, I hope you weren't waiting
on me!

Note that clang works fine on .hpp headers; the problem here was an unrelated one with
canonicalized paths.  Is that the problem that you're seeing too?

Reported by csilvers on 2011-04-15 20:27:36

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

I believe so. I can compile the code with clang. Is there a way to trigger the analysis
on *.hpp as of r130? 

Reported by wing1127aishi on 2011-04-17 02:41:49

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

.hpp files "associated" with a .cpp file -- with the same basename -- should be analyzed
automatically.  If that code isn't working right, you can use the --check_also flag
to include-what-you-use to manually specify a file to analyze.

Reported by csilvers on 2011-04-17 04:42:43

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

Sorry for the drive-by-patch, I got busy with other things. I'll try to circle back
and update this patch within the next week or so.

Reported by tonyg@chromium.org on 2011-04-18 13:47:38

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

To Craig

*.hpp associates with each *.cpp files within the same directory are not analysed automatically.
Only suggestions for *.cpp are generated. I will try the --check_also option shortly.


Reported by wing1127aishi on 2011-04-28 21:51:39

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

--check_also should work.  Feel free to make a patch to treat .hpp the same as .h --
I'm sure there are several places where '.h' is hard-coded into iwyu.

Reported by csilvers on 2011-04-28 22:22:40

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

Hey tony, just a gentle ping. :-)

Reported by csilvers on 2011-05-04 18:39:32

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

Sorry again for the delay. Here's a patch which fixes issue 5 and 16 for Chromium and
WebKit.

Both of the new tests fail without this patch and pass now. All other tests continue
to pass.

The IWYU results for these projects are now very sensible.

Reported by tonyg@chromium.org on 2011-05-06 15:15:12


- _Attachment: [issue_5_and_16.patch](https://storage.googleapis.com/google-code-attachments/include-what-you-use/issue-16/comment-19/issue_5_and_16.patch)_
@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

Thanks for the patch!  I've been a bit overwhelmed lately, but will look at this when
I get a chance, hopefully this week but maybe the next.

Reported by csilvers on 2011-05-09 19:57:21

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

OK, taking a look now.  Tony, can you sign the CLA if need be?
   http://code.google.com/legal/individual-cla-v1.0.html

Thanks

Reported by csilvers on 2011-05-15 23:52:27

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

Hey Tony, looked at the patch, and it looks good.  There are a couple of things I don't
understand fully yet:

1) What does your canonicalization code do for something like '-I ../foo'?  It seems
like there the 'startswith(GetCWD())' test will fail.  Is that ok?

2) realpath resolves symlinks.  Is that ok?  I'm guessing no -- the GetCWD() test (which
doesn't resolve symlinks, I don't think) will never pass if you're compiling from a
directory that's a symlink.  Also realpath() is a BSD-ism.  Dunno if that matters for
clang or not.

Is there no llvm command that does the equivalent?  If not, I think your best bet may
be to remove '/foo/../' substrings manually.  If the result starts with .., you can
start working on GetCWD() as well (assuming that's the right thing to do).

3)
+      // Some projects include user paths via -I which causes them to appear
+      // like system paths to Clang. So we only trust that they are system paths

Can you explain this a bit more?  -I is for user paths.  -isystem is for passing in
system paths.  Does clang really mark -I as a system path?  That seems hard to believe.
 I'd like to understand this better.  I'm not really comfortable using relative vs
absolute to distinguish user from system paths; at google, for instance, we have some
system include-dirs that are specified as a relative path.

4) +  errs() << internal::PrintableDiffs(CanonicalizeFilePath(GetFilePath(file_)),

Can you elaborate a bit on this?  I guess it's for consistency between unix and windows
runs?  If so, I'd suggest moving the CanonicalizeFilePath call to inside PrintableDiffs,
with a comment like
    // Make the output here consistent between unix and windows

And two suggested changes (one tiny, one not-tiny):

A) You added a map of per-test includes to the testing framework, but I think it would
be better to have a map of per-test clang flags instead.  You'd just change your map
values like '../foo' to '-I../foo' instead.

B) The firstline files on your test files are wrong.  (I know, this trips me up all
the time also, and I don't see the value in this case, but it seems to be clang convention.)

Thanks again for taking this on!

Reported by csilvers on 2011-05-16 00:15:51

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

> 3)
> +      // Some projects include user paths via -I which causes them to appear
> +      // like system paths to Clang. So we only trust that they are system paths
>
> Can you explain this a bit more?  -I is for user paths.  -isystem is for passing
in system paths.  Does clang really mark -I as a system path?  That seems hard to believe.
 I'd like to understand this better.  I'm not really comfortable using relative vs
absolute to distinguish user from system paths; at google, for instance, we have some
system include-dirs that are specified as a relative path.

Let's focus first on just this since it is really the core issue for me -- once we
solve it, I'll address the others.

Here's an example compilation command for Chrome:

/usr/local/google/code/chromium/src/third_party/llvm-build/Release+Asserts/bin/include-what-you-use
-x objective-c++ -arch i386 -fmessage-length=0 -pipe -Wno-trigraphs -fno-exceptions
-fno-rtti -O3 -Werror -Wnewline-eof -DCHROMIUM_BUILD -DENABLE_REMOTING=1 -DENABLE_P2P_APIS=1
-DENABLE_GPU=1 -DENABLE_EGLIMAGE=1 -DSK_BUILD_NO_IMAGE_ENCODE -DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"
-DGR_AGGRESSIVE_SHADER_OPTS=1 -DUNIT_TEST -DGTEST_HAS_RTTI=0 -DGTEST_USE_OWN_TR1_TUPLE=1
-D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -isysroot
/Developer/SDKs/MacOSX10.5.sdk -fvisibility=hidden -fvisibility-inlines-hidden -fno-threadsafe-statics
-mmacosx-version-min=10.5 -gdwarf-2 -Wall -Wendif-labels -Wextra -Wno-unused-parameter
-Wno-missing-field-initializers -Wheader-hygiene -Wno-char-subscripts -Wno-unused-function
-Wno-unnamed-type-template-args -F/usr/local/google/code/chromium/src/clang/Release
-F/Developer/SDKs/MacOSX10.5.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks
-I/usr/local/google/code/chromium/src/clang/Release/include -I../gpu -I.. -I../skia/config
-I../third_party/skia/include/config -I../third_party/skia/include/core -I../third_party/skia/include/effects
-I../third_party/skia/include/pdf -I../third_party/skia/include/gpu -I../third_party/skia/include/ports
-I../third_party/skia/gpu/include -I../skia/ext -I../testing/gmock/include -I../testing/gtest/include
-I../third_party/npapi -I../third_party/npapi/bindings -I/usr/local/google/code/chromium/src/clang/DerivedSources/Release/webkit
-I/usr/local/google/code/chromium/src/clang/obj/webkit.build/Release/test_shell_common.build/DerivedSources/i386
-I/usr/local/google/code/chromium/src/clang/obj/webkit.build/Release/test_shell_common.build/DerivedSources
-fno-strict-aliasing -fobjc-exceptions -c /Volumes/Code/chromium/src/webkit/tools/test_shell/test_shell_mac.mm
-o /usr/local/google/code/chromium/src/clang/obj/webkit.build/Release/test_shell_common.build/Objects-normal/i386/test_shell_mac.o

Notice the many paths included via -I and none via -isystem. If I print entry->getName(),
in each loop of ComputeHeaderSearchPaths(), here's what I get:

System: ../gpu
System: ..
System: ../skia/config
System: ../third_party/skia/include/config
System: ../third_party/skia/include/core
System: ../third_party/skia/include/effects
System: ../third_party/skia/include/pdf
System: ../third_party/skia/include/gpu
System: ../third_party/skia/include/ports
System: ../third_party/skia/gpu/include
System: ../skia/ext
System: ../testing/gmock/include
System: ../testing/gtest/include
System: ../third_party/npapi
System: ../third_party/npapi/bindings
System: /usr/local/google/code/chromium/src/clang/DerivedSources/Release/webkit
System: /Developer/SDKs/MacOSX10.5.sdk/usr/include/c++/4.2.1
System: /Developer/SDKs/MacOSX10.5.sdk/usr/include/c++/4.2.1/i686-apple-darwin10
System: /Developer/SDKs/MacOSX10.5.sdk/usr/include/c++/4.2.1/backward
System: /Volumes/Code/chromium/src/third_party/llvm-build/Release+Asserts/bin/../lib/clang/3.0/include
System: /Developer/SDKs/MacOSX10.5.sdk/usr/include
User: ../gpu
User: ..
User: ../skia/config
User: ../third_party/skia/include/config
User: ../third_party/skia/include/core
User: ../third_party/skia/include/effects
User: ../third_party/skia/include/pdf
User: ../third_party/skia/include/gpu
User: ../third_party/skia/include/ports
User: ../third_party/skia/gpu/include
User: ../skia/ext
User: ../testing/gmock/include
User: ../testing/gtest/include
User: ../third_party/npapi
User: ../third_party/npapi/bindings
User: /usr/local/google/code/chromium/src/clang/DerivedSources/Release/webkit
User: /Developer/SDKs/MacOSX10.5.sdk/usr/include/c++/4.2.1
User: /Developer/SDKs/MacOSX10.5.sdk/usr/include/c++/4.2.1/i686-apple-darwin10
User: /Developer/SDKs/MacOSX10.5.sdk/usr/include/c++/4.2.1/backward
User: /Volumes/Code/chromium/src/third_party/llvm-build/Release+Asserts/bin/../lib/clang/3.0/include
User: /Developer/SDKs/MacOSX10.5.sdk/usr/include

According to http://clang.llvm.org/doxygen/HeaderSearch_8h_source.html#l00296, system_dir_begin():system_dir_end()
should iterate through only system dirs and search_dir_begin():search_dir_end() should
loop through all. So I'd expect to see something more like:

System: /Developer/SDKs/MacOSX10.5.sdk/usr/include/c++/4.2.1
System: /Developer/SDKs/MacOSX10.5.sdk/usr/include/c++/4.2.1/i686-apple-darwin10
System: /Developer/SDKs/MacOSX10.5.sdk/usr/include/c++/4.2.1/backward
System: /Volumes/Code/chromium/src/third_party/llvm-build/Release+Asserts/bin/../lib/clang/3.0/include
System: /Developer/SDKs/MacOSX10.5.sdk/usr/include
User: ../gpu
User: ..
User: ../skia/config
User: ../third_party/skia/include/config
User: ../third_party/skia/include/core
User: ../third_party/skia/include/effects
User: ../third_party/skia/include/pdf
User: ../third_party/skia/include/gpu
User: ../third_party/skia/include/ports
User: ../third_party/skia/gpu/include
User: ../skia/ext
User: ../testing/gmock/include
User: ../testing/gtest/include
User: ../third_party/npapi
User: ../third_party/npapi/bindings
User: /usr/local/google/code/chromium/src/clang/DerivedSources/Release/webkit
User: /Developer/SDKs/MacOSX10.5.sdk/usr/include/c++/4.2.1
User: /Developer/SDKs/MacOSX10.5.sdk/usr/include/c++/4.2.1/i686-apple-darwin10
User: /Developer/SDKs/MacOSX10.5.sdk/usr/include/c++/4.2.1/backward
User: /Volumes/Code/chromium/src/third_party/llvm-build/Release+Asserts/bin/../lib/clang/3.0/include
User: /Developer/SDKs/MacOSX10.5.sdk/usr/include

So, for Chrome+WebKit I was able to get reasonable output by using the (flawed) absolute
path heuristic. But if that doesn't work for google source (and probably other projects),
obviously we can't do that. Maybe the Clang patch Paul mentions in issue 5 didn't quite
work as expected or perhaps this is another upstream Clang bug? Either way, maybe it
shouldn't be hacked around in iwyu.

I would like to continue helping to get iwyu working w/ Chrome+WebKit, but realistically,
I'm not going to have the time to start hacking on Clang itself. Please let me know
if you have any ideas for solving this, otherwise I'll just keep using this patch off-tree
for WebKit cleanup work (cover bug: https://bugs.webkit.org/show_bug.cgi?id=52451).

Reported by tonyg@chromium.org on 2011-05-16 09:32:32

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

Hi Craig, Tony,

> > Can you explain this a bit more?  -I is for user paths.  -isystem is for passing
in system paths.  Does clang really mark -I as a system path?  That seems hard to believe.

> Maybe the Clang patch Paul mentions in issue 5 didn't quite work as expected or perhaps
this is another upstream Clang bug? Either way, maybe it shouldn't be hacked around
in iwyu.

I've investigated this a bit more, and realised that Clang makes a quite subtle distinction
between -iquote, -isystem and -I which I hadn't appreciated when submitting the original
patch.

Running Clang with the '-v' option gives a good demonstration of this behaviour. For
example I see this:

C:\dev>"\Program Files (x86)\LLVM\bin\clang.exe" -c foo.cpp -Iblah -v
<snipped>
#include "..." search starts here:
#include <...> search starts here:
 blah
 C:/Program Files (x86)/LLVM/bin/../lib/clang/3.0/include
 c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\include
 C:\Program Files (x86)\Microsoft SDKs\Windows\v7.0A\\include
End of search list.

So '-Iblah' is being treated by Clang as a 'system' search path (as far as IWYU sees).
Compare to:

C:\dev>"\Program Files (x86)\LLVM\bin\clang.exe" -c foo.cpp -iquoteblah -v
<snipped>
#include "..." search starts here:
 blah
#include <...> search starts here:
 C:/Program Files (x86)/LLVM/bin/../lib/clang/3.0/include
 c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\include
 C:\Program Files (x86)\Microsoft SDKs\Windows\v7.0A\\include
End of search list.

Which treats 'blah' as a 'user' path, and this:

C:\dev>"\Program Files (x86)\LLVM\bin\clang.exe" -c foo.cpp -isystemblah -v
<snipped>
#include "..." search starts here:
#include <...> search starts here:
 blah
 C:/Program Files (x86)/LLVM/bin/../lib/clang/3.0/include
 c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\include
 C:\Program Files (x86)\Microsoft SDKs\Windows\v7.0A\\include
End of search list.

Which treats 'blah' as if it were specified with -I.


I think the code responsible for this behaviour is in Clang's ParseHeaderSearchArgs:

  // Add -I... and -F... options in order.
  for (arg_iterator it = Args.filtered_begin(OPT_I, OPT_F),
         ie = Args.filtered_end(); it != ie; ++it)
    Opts.AddPath((*it)->getValue(Args), frontend::Angled, true,
                 /*IsFramework=*/ (*it)->getOption().matches(OPT_F), true);

'frontend::Angled' is a member of this enum:

  enum IncludeDirGroup {
    Quoted = 0,     ///< `#include ""` paths. Thing `gcc -iquote`.
    Angled,         ///< Paths for both `#include ""` and `#include <>`. (`-I`)
    System,         ///< Like Angled, but marks system directories.
    CXXSystem,      ///< Like System, but only used for C++.
    After           ///< Like System, but searched after the system directories.
  };

Clang's HeaderSearch tracks 'SystemDirIdx' as the boundary between Quoted and Angled
(so that #include "" skips Quoted, but searches Angled, System etc.) It seems like
we probably need to distinguish between Angled and System. I'm happy to look into working
on a patch for Clang to expose this.

Regards,
Paul


Reported by paul.holden on 2011-05-16 22:11:34

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

Paul, that soudns great -- can you work on getting that exposed via clang?  Once it
is, we can modify iwyu to do the right thing.

Ideally, we'd just have an iterator over HeaderSearchOption::Entry's.  It looks like
HeaderSearchOption::UserEntries is public -- I'm a little surprised by that, honestly
-- so maybe just passing in the HeaderSearchOptions struct directly, and working on
that, is the easiest solution.  Is that possible?

Reported by csilvers on 2011-05-17 00:36:47

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

One thing I've just spotted is that DirectoryLookup provides a isUserSupplied() method.
Perhaps this is precisely what we need? i.e. we could just do this?:

  search_path_map.insert(make_pair(entry->getName(),
    entry->isUserSupplied() ?
      HeaderSearchPath::kUserPath :
      HeaderSearchPath::kSystemPath));

I'll investigate this a bit more this evening.

Reported by paul.holden on 2011-05-17 07:03:06

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

The attached patch seems to work for me, and simplifies the existing code somewhat.

Before:

C:\dev> llvm_build\bin\Debug\include-what-you-use.exe hello.cpp -Iblah
Setting verbose-level to 6
Search path: c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\include (system)
Search path: C:\Program Files (x86)\Microsoft SDKs\Windows\v7.0A\\include (system)
Search path: C:/dev/llvm_build/bin/Debug/../lib/clang/3.0/include (system)
Search path: blah (system)

After applying the patch:

C:\dev> llvm_build\bin\Debug\include-what-you-use.exe hello.cpp -Iblah
Setting verbose-level to 6
Search path: c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\include (system)
Search path: C:\Program Files (x86)\Microsoft SDKs\Windows\v7.0A\\include (system)
Search path: C:/dev/llvm_build/bin/Debug/../lib/clang/3.0/include (system)
Search path: blah (user)

If it looks good I'll apply.

Reported by paul.holden on 2011-05-17 21:59:35


- _Attachment: [system_dir_fix.patch](https://storage.googleapis.com/google-code-attachments/include-what-you-use/issue-16/comment-27/system_dir_fix.patch)_
@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

I don't think this is quite right -- "-isystem foo" is user-supplied, but still a system
directory.

I think user-supplied is the wrong thing to look at here -- we really want to look
at the IncludeDirGroup enum, I think.

Reported by csilvers on 2011-05-17 22:21:17

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

Whoops, I'd not thought of that. I'll work on exposing this via Clang. 

Reported by paul.holden on 2011-05-18 06:59:17

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

This is my patch to fix up Clang's HeaderSearch::system_dir_begin/system_dir_end to
work as we'd expect. With this IWYU's ComputeHeaderSearchPaths() behaves correctly
without further changes.

Here's some sample output:

C:\dev\test_iwyu>include-what-you-use test.cpp  -Ifoo -iquotebar -isystembaz -v
<snip>
#include "..." search starts here:
 bar
#include <...> search starts here:
 foo
 baz
 c:/Program Files (x86)/LLVM/bin/../lib/clang/3.0/include
 c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\include
 C:\Program Files (x86)\Microsoft SDKs\Windows\v7.0A\\include
End of search list.

Search path: c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\include (system)
Search path: C:\Program Files (x86)\Microsoft SDKs\Windows\v7.0A\\include (system)
Search path: c:/Program Files (x86)/LLVM/bin/../lib/clang/3.0/include (system)
Search path: bar (user)
Search path: baz (system)
Search path: foo (user)


IWYU now correctly sees 'foo' as a user dir, and 'baz' as a system dir. 

Tony - are you in the position to verify that this fixes the issue you raised in comment
23? If it fixes your issue I'll submit it to the Clang mailing list and see if I can
get it applied.


Reported by paul.holden on 2011-05-22 21:54:47


- _Attachment: [differentiate_angle_and_system_dirs.patch](https://storage.googleapis.com/google-code-attachments/include-what-you-use/issue-16/comment-30/differentiate_angle_and_system_dirs.patch)_
@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

> Tony - are you in the position to verify that this fixes the issue you raised in comment
23? If it fixes your issue I'll submit it to the Clang mailing list and see if I can
get it applied.

Yes. Thank you. That works perfectly. If you remember, could you please ping this bug
when your patch lands?

Craig, I'll update the patch to address the rest of your original comments.

Reported by tonyg@google.com on 2011-05-23 10:58:54

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

Great! -- thanks for the clang fix.  Hopefully that will allow us to resolve this soon.

Reported by csilvers on 2011-05-24 03:25:58

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

Phew - I'm glad this is finally working as expected.

I submitted the patch to cfe-commits last night. Last time around it took 1-2 weeks
to apply - hopefully it'll be a little quicker this time.

Reported by paul.holden on 2011-05-24 06:46:55

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

Or....apparently this was applied in r131955 :)

Reported by paul.holden on 2011-05-24 06:52:50

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

Saw a message that it was just applied!

Remind me what the next step is? :-)

Reported by csilvers on 2011-05-24 06:52:55

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

Gentle ping on this -- is there something I should be doing now?

Reported by csilvers on 2011-06-09 00:39:38

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

In case this issue was not reproduced on maintainer's environment, with llvm and iwyu
trunk, iwyu still only produce output for *.cpp.

Reported by wing1127aishi on 2011-06-09 00:56:07

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

Well, I've always gotten output for .h files with iwyu. :-)  If you're not, you'll have
to give more specifics on to reproduce the behavior.  The files you ran iwyu over,
and the exact iwyu command you ran, would be very helpful.

Reported by csilvers on 2011-06-09 01:16:52

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

ls -l in the source directory gives

-rw-r--r--    1 leon.sit Administ     2365 May 31 09:39 basevisitor.cpp
-rw-r--r--    1 leon.sit Administ     9101 May 31 09:39 basevisitor.hpp
-rw-r--r--    1 leon.sit Administ     2030 May 31 09:39
creditratingparser.cpp
-rw-r--r--    1 leon.sit Administ     2596 May 31 09:39
creditratingparser.hpp
-rw-r--r--    1 leon.sit Administ       29 May 31 09:39 dailyevaluator.cpp
-rw-r--r--    1 leon.sit Administ     3230 Jun  7 15:08 dailyevaluator.hpp
-rw-r--r--    1 leon.sit Administ      439 May 31 09:39
dailyevaluatortrait.hpp
-rw-r--r--    1 leon.sit Administ      355 May 31 09:39 dailyupdate.cpp
-rw-r--r--    1 leon.sit Administ      627 May 31 09:39 dailyupdate.hpp
-rw-r--r--    1 leon.sit Administ      737 May 31 09:39 dailyupdate_u.hpp
-rw-r--r--    1 leon.sit Administ     1031 May 31 09:39 error.hpp
-rw-r--r--    1 leon.sit Administ       24 May 31 09:39 evaluator.cpp
-rw-r--r--    1 leon.sit Administ     5406 Jun  7 15:08 evaluator.hpp
-rw-r--r--    1 leon.sit Administ      291 May 31 09:39 evaluatortrait.hpp
-rw-r--r--    1 leon.sit Administ       22 May 31 09:39 factory.cpp
-rw-r--r--    1 leon.sit Administ     3247 May 31 09:39 factory.hpp
-rw-r--r--    1 leon.sit Administ       41 May 31 09:39
fileProgressBarWrapper.cpp
-rw-r--r--    1 leon.sit Administ     1515 May 31 09:39
fileProgressBarWrapper.hpp
-rw-r--r--    1 leon.sit Administ      158 May 31 09:39 globalsettings.cpp
-rw-r--r--    1 leon.sit Administ     2278 May 31 09:39 globalsettings.hpp
-rw-r--r--    1 leon.sit Administ     1796 May 31 09:39 instrumentType.hpp
-rw-r--r--    1 leon.sit Administ     8360 May 31 09:39
instrumentupdater.hpp
-rw-r--r--    1 leon.sit Administ     2754 May 31 09:39 json_demo.cpp
-rw-r--r--    1 leon.sit Administ     5137 Jun  7 15:08 jsonutililty.cpp
-rw-r--r--    1 leon.sit Administ     2892 Jun  7 15:08 jsonutility.hpp
-rw-r--r--    1 leon.sit Administ     2616 Jun  7 15:08 labelconstant.hpp
-rw-r--r--    1 leon.sit Administ     4750 May 31 09:39 makeBond.cpp
-rw-r--r--    1 leon.sit Administ    11780 May 31 09:39 makeBond.hpp
-rw-r--r--    1 leon.sit Administ       31 May 31 09:39 makeCurrencySwap.cpp
-rw-r--r--    1 leon.sit Administ     8477 May 31 09:39 makeCurrencySwap.hpp
-rw-r--r--    1 leon.sit Administ     3079 May 31 09:39
makeInterestRateIndex.cpp
-rw-r--r--    1 leon.sit Administ     8385 May 31 09:39
makeInterestRateIndex.hpp
-rw-r--r--    1 leon.sit Administ     4114 May 31 09:39
makeSwaptionVolatilityStructure.cpp
-rw-r--r--    1 leon.sit Administ     2295 May 31 09:39
makeSwaptionVolatilityStructure.hpp
-rw-r--r--    1 leon.sit Administ     5724 May 31 09:39
makeYieldTermStructure.cpp
-rw-r--r--    1 leon.sit Administ     2706 May 31 09:39
makeYieldTermStructure.hpp
-rw-r--r--    1 leon.sit Administ       74 May 31 09:39
makevanillaoption.hpp
-rw-r--r--    1 leon.sit Administ     7215 May 31 09:39 market.cpp
-rw-r--r--    1 leon.sit Administ    12793 May 31 09:39 market.hpp
-rw-r--r--    1 leon.sit Administ    94146 Jun  8 20:02 marketUtility.cpp
-rw-r--r--    1 leon.sit Administ     7958 Jun  1 10:34 marketUtility.hpp
-rw-r--r--    1 leon.sit Administ       30 May 31 09:39 marketpredicate.cpp
-rw-r--r--    1 leon.sit Administ     4146 May 31 09:39 marketpredicate.hpp
-rw-r--r--    1 leon.sit Administ     7780 May 31 09:39
marketpredicateoperator.hpp
-rw-r--r--    1 leon.sit Administ     2900 Jun  7 15:08 portfolio.cpp
-rw-r--r--    1 leon.sit Administ    10900 Jun  7 15:08 portfolio.hpp
-rw-r--r--    1 leon.sit Administ     3365 Jun  7 15:08
quarterlyevaluator.hpp
-rw-r--r--    1 leon.sit Administ      365 May 31 09:39
quarterlyevaluatortrait.hpp
-rw-r--r--    1 leon.sit Administ      456 May 31 09:39
quarterlyupdate-inl.hpp
-rw-r--r--    1 leon.sit Administ      654 May 31 09:39 quarterlyupdate.hpp
-rw-r--r--    1 leon.sit Administ      111 May 31 09:39 shock.cpp
-rw-r--r--    1 leon.sit Administ    18262 May 31 11:20 shock.hpp
-rw-r--r--    1 leon.sit Administ       31 May 31 09:39 shockedevaluator.cpp
-rw-r--r--    1 leon.sit Administ      771 May 31 09:39 shockedevaluator.hpp
-rw-r--r--    1 leon.sit Administ     2602 May 31 09:39 shockfactory.cpp
-rw-r--r--    1 leon.sit Administ     2099 May 31 09:39 shockfactory.hpp
-rw-r--r--    1 leon.sit Administ     4718 May 31 09:39
shockmarketbuilder.cpp
-rw-r--r--    1 leon.sit Administ     3511 May 31 09:39
shockmarketbuilder.hpp
-rw-r--r--    1 leon.sit Administ      736 May 31 09:39 shocktableparser.cpp
-rw-r--r--    1 leon.sit Administ     3912 May 31 09:39 shocktableparser.hpp
-rw-r--r--    1 leon.sit Administ      734 May 31 09:39 shocktask.hpp
-rw-r--r--    1 leon.sit Administ     1658 May 31 09:39 shocktemplate.cpp
-rw-r--r--    1 leon.sit Administ     3506 May 31 09:39 shocktemplate.hpp
-rw-r--r--    1 leon.sit Administ       24 May 31 09:39 tasklists.cpp
-rw-r--r--    1 leon.sit Administ     5812 Jun  7 15:08 tasklists.hpp
-rw-r--r--    1 leon.sit Administ       31 May 31 09:39 updatedispatcher.cpp
-rw-r--r--    1 leon.sit Administ     6390 Jun  7 15:08 updatedispatcher.hpp
-rw-r--r--    1 leon.sit Administ      792 May 31 09:39
updatevisitor-inl.hpp
-rw-r--r--    1 leon.sit Administ      704 May 31 09:39 updatevisitor.cpp
-rw-r--r--    1 leon.sit Administ     1674 May 31 09:39 updatevisitor.hpp
-rw-r--r--    1 leon.sit Administ    12911 Jun  7 15:08 utility.cpp
-rw-r--r--    1 leon.sit Administ     3122 Jun  7 15:08 utility.hpp
-rw-r--r--    1 leon.sit Administ     1958 May 31 09:39 yieldshockparser.cpp
-rw-r--r--    1 leon.sit Administ     2748 May 31 09:39 yieldshockparser.hpp

so *.hpp and *.cpp are in the same directory. I just run the commend with

make -k CXX=[path_to_include_what_you_use]

in the source directory. Nothing really special. I will try to reproduce
with reduced settings at some point.

Reported by wing1127aishi on 2011-06-09 01:40:26

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

Oh right, your issue was with the extension '.hpp' instead of '.h'.  I don't know of
any patches that attempt to address that problem.  iwyu does not consider .hpp to be
a header file.  This should be easy to fix, if you want to draw up a patch.  You should
make a new bug report for it, though; it's different from this one.

Reported by csilvers on 2011-06-09 01:48:10

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

I will give it a try but no guarantee. What file should I look at first?

Reported by wing1127aishi on 2011-06-09 02:07:29

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

Tony, just checking back with this again.  I think we're back to the feedback on comment
22.  (3) may or may not be resolved here (I think the patch will probably need to change
to take paul holden's clang changes into account), but the others are still open as
far as I know.  Have you had time to look into this recently?

Reported by csilvers on 2011-07-12 01:43:11

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

I am trying to use this with the libreoffice code and I noticed that I'm not getting
output from any hxx files and most cxx files. Any code pointers for how I might look
into this?

Reported by augsod on 2011-11-22 06:16:29

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

augsod: can you be more specific?  What is the exact command you're running, and what
output are you getting?  What version of iwyu are you using (svn trunk?)  iwyu should
work fine on .hxx and .cxx files these days.  Perhaps they're not compiling cleanly
under clang or some such?

Reported by csilvers on 2011-11-22 16:17:20

@vsapsai
Copy link
Contributor Author

@vsapsai vsapsai commented Jun 15, 2015

FYI everything works fine for me with newer clang.

Reported by augsod on 2012-06-07 21:04:22

@kimgr
Copy link
Contributor

@kimgr kimgr commented Jun 12, 2016

I think this is the same issue as is being fixed by #294. I haven't found many small repros in this issue, so maybe it's just time to close it when #294 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.