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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:54
+  /// A verbatim header spelling, a string quoted with <> or "" that can be
+  /// #included directly
+  Header(StringRef VerbatimSpelling) : Storage(VerbatimSpelling) {}
----------------
nit: `.` at the end of comment


================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:67
+  }
+  return Results;
+}
----------------
can you actually put an `llvm_unreachable` here?


================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:55
+    if (!VerbatimSpelling.empty())
+      return {{VerbatimSpelling}};
+
----------------
hokein wrote:
> kadircet wrote:
> > what about exporters here?
> based on our previous discussion, we decided to initially treat the spelling header in the IWYU private pragma as the final public header, so no need to do the exporters here.
yeah SG, can you put a comment about that though? saying `// Note that we might need to look for exporters in this case as well.`


================
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()))
----------------
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?


================
Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:135
+
+TEST(FindIncludeHeaders, IWYU) {
+  TestInputs Inputs;
----------------
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 {};`


================
Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:116
+
+  EXPECT_THAT(walk(AST),
+              UnorderedElementsAre(
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137320