[PATCH] D137320: [include-cleaner] Initial version for the "Location=>Header" step

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:58
+    Results = {{FE}};
+    // FIXME: compute a closure of exporter headers.
+    for (const auto *Export : PI.getExporters(FE, SM.getFileManager()))
----------------
kadircet wrote:
> hokein wrote:
> > kadircet wrote:
> > > not sure i follow the fixme here, are you suggesting we should also compute transitive exporters?
> > Yeah, rephrased the FIXME.
> i see, i am not sure if that's desired though.
> 
> e.g. if we have `foo.h` exporting `bar.h` exporting `baz.h`, I don't think `baz.h` counts (or should count) as an alternative to `foo.h`. do we have information proving otherwise?
The classical IWYU tool has this semantic, see the nested_export test in https://github.com/include-what-you-use/include-what-you-use/commit/9945e543d894e90fab5ebd0b685cabd5aa8dd21f.


================
Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:135
+
+TEST(FindIncludeHeaders, IWYU) {
+  TestInputs Inputs;
----------------
kadircet wrote:
> you can still use the same fixture here, if you want a different name you can always do: `using FindIncludeHeaders = WalkUsedTest;` or `class FindIncludeHeaders : public WalkUsedTest {};`
There is not much code being shared between these two tests (only the MakeAction, walk() is not used) -- the FindIncludeHeaders test is simpler than the WalkUsedTest, we don't need to build an AST or traverse the AST nodes.


================
Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:116
+
+  EXPECT_THAT(walk(AST),
+              UnorderedElementsAre(
----------------
kadircet wrote:
> hokein wrote:
> > kadircet wrote:
> > > can we use `findIncludeHeaders` directly?
> > > 
> > > that way we don't need to set up a referencing file, e.g. you can directly create a SourceLocation inside `private.h` (in this case even the decl is not important). 
> > > 
> > > 
> > > (same for the export test)
> > the intention was to do an end-to-end test of the walkUsed API. 
> > 
> > but you're right, we should have a unittest for `findIncludeHeaders`, added one, and simplified the end-to-end walkUsed test. 
> > the intention was to do an end-to-end test of the walkUsed API.
> 
> We can update the `Basic` test above for that purpose (and turn it into a smoke test instead).
> 
> > but you're right, we should have a unittest for findIncludeHeaders, added one, and simplified the end-to-end walkUsed test.
> 
> Since these are all trying to test specific IWYU pragma behaviour, I still think it's better to test them in isolation, both for test brevity but also for debuggability in the future when something breaks.
> Also making sure majority of the tests are smaller is good for incremental progress as well, when we update ast walking logic, we shouldn't be updating these tests for whatever reasons (+ arguments around test times).
ok, moved to WalkUsedTest.Basic. now the test is just a smoke test for IWYU pragma.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137320/new/

https://reviews.llvm.org/D137320