search

engmsaleh / include-what-you-use

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

Analyze headers #16

Open GoogleCodeExporter opened 9 years ago

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

Original issue reported on code.google.com by tonyg@chromium.org on 5 Mar 2011 at 12:07

GoogleCodeExporter commented 9 years ago 
iwyu analyzes .h files.  What is making you think it doesn't?

Original comment by csilv...@gmail.com on 5 Mar 2011 at 12:16

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

Original comment by tonyg@chromium.org on 5 Mar 2011 at 12:19

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

Original comment by csilv...@gmail.com on 5 Mar 2011 at 12:27

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

Original comment by tonyg@chromium.org on 14 Mar 2011 at 10:32

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

Original comment by csilv...@gmail.com on 14 Mar 2011 at 10:55

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

Original comment by tonyg@chromium.org on 14 Mar 2011 at 11:22

Attachments:

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

Original comment by csilv...@gmail.com on 15 Mar 2011 at 12:00

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

Original comment by tonyg@chromium.org on 15 Mar 2011 at 6:09

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

Original comment by tonyg@chromium.org on 15 Mar 2011 at 9:07

Attachments:

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

Original comment by csilv...@gmail.com on 15 Mar 2011 at 9:48

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

Original comment by wing1127aishi@gmail.com on 15 Apr 2011 at 6:50

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

Original comment by csilv...@gmail.com on 15 Apr 2011 at 8:27

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

Original comment by wing1127aishi@gmail.com on 17 Apr 2011 at 2:41

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

Original comment by csilv...@gmail.com on 17 Apr 2011 at 4:42

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

Original comment by tonyg@chromium.org on 18 Apr 2011 at 1:47

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

Original comment by wing1127aishi@gmail.com on 28 Apr 2011 at 9:51

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

Original comment by csilv...@gmail.com on 28 Apr 2011 at 10:22

GoogleCodeExporter commented 9 years ago 
Hey tony, just a gentle ping. :-)

Original comment by csilv...@gmail.com on 4 May 2011 at 6:39

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

Original comment by tonyg@chromium.org on 6 May 2011 at 3:15

Attachments:

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

Original comment by csilv...@gmail.com on 9 May 2011 at 7:57

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

Original comment by csilv...@gmail.com on 15 May 2011 at 11:52

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

Original comment by csilv...@gmail.com on 16 May 2011 at 12:15

GoogleCodeExporter commented 9 years ago 
> 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/i
nclude-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.f
ramework/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_co
mmon.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/cla
ng/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/cla
ng/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/cla
ng/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/cla
ng/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).

Original comment by tonyg@chromium.org on 16 May 2011 at 9:32

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

Original comment by paul.hol...@gmail.com on 16 May 2011 at 10:11

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

Original comment by csilv...@gmail.com on 17 May 2011 at 12:36

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

Original comment by paul.hol...@gmail.com on 17 May 2011 at 7:03

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

Original comment by paul.hol...@gmail.com on 17 May 2011 at 9:59

Attachments:

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

Original comment by csilv...@gmail.com on 17 May 2011 at 10:21

GoogleCodeExporter commented 9 years ago 
Whoops, I'd not thought of that. I'll work on exposing this via Clang.

Original comment by paul.hol...@gmail.com on 18 May 2011 at 6:59

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

Original comment by paul.hol...@gmail.com on 22 May 2011 at 9:54

Attachments:

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

Original comment by tonyg@google.com on 23 May 2011 at 10:58

GoogleCodeExporter commented 9 years ago 
Great! -- thanks for the clang fix.  Hopefully that will allow us to resolve 
this soon.

Original comment by csilv...@gmail.com on 24 May 2011 at 3:25

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

Original comment by paul.hol...@gmail.com on 24 May 2011 at 6:46

GoogleCodeExporter commented 9 years ago 
Or....apparently this was applied in r131955 :)

Original comment by paul.hol...@gmail.com on 24 May 2011 at 6:52

GoogleCodeExporter commented 9 years ago 
Saw a message that it was just applied!

Remind me what the next step is? :-)

Original comment by csilv...@gmail.com on 24 May 2011 at 6:52

GoogleCodeExporter commented 9 years ago 
Gentle ping on this -- is there something I should be doing now?

Original comment by csilv...@gmail.com on 9 Jun 2011 at 12:39

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

Original comment by wing1127aishi@gmail.com on 9 Jun 2011 at 12:56

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

Original comment by csilv...@gmail.com on 9 Jun 2011 at 1:16

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

Original comment by wing1127aishi@gmail.com on 9 Jun 2011 at 1:40

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

Original comment by csilv...@gmail.com on 9 Jun 2011 at 1:48

GoogleCodeExporter commented 9 years ago 
I will give it a try but no guarantee. What file should I look at first?

Original comment by wing1127aishi@gmail.com on 9 Jun 2011 at 2:07

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

Original comment by csilv...@gmail.com on 12 Jul 2011 at 1:43

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

Original comment by augsod@gmail.com on 22 Nov 2011 at 6:16

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

Original comment by csilv...@gmail.com on 22 Nov 2011 at 4:17

GoogleCodeExporter commented 9 years ago 
FYI everything works fine for me with newer clang.

Original comment by augsod@gmail.com on 7 Jun 2012 at 9:04