[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/include/clang-include-cleaner/Types.h:63
+// FIXME: use a real Class!
+using SymbolLocation = std::variant<SourceLocation, tooling::stdlib::Symbol>;
+
----------------
kadircet wrote:
> let's move this to `AnalysisInternal.h`, as it isn't used in any of the public apis.
I'm not quite sure about this --the SymbolLocation is an important concept and API, even though it is not used in public APIs. Moved anyway.



================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:55
+    if (!VerbatimSpelling.empty())
+      return {{VerbatimSpelling}};
+
----------------
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.


================
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:
> not sure i follow the fixme here, are you suggesting we should also compute transitive exporters?
Yeah, rephrased the FIXME.


================
Comment at: clang-tools-extra/include-cleaner/lib/AnalysisInternal.h:49
+// FIXME: expose signals
+llvm::SmallVector<Header> findIncludeHeaders(const SymbolLocation &Loc,
+                                             const SourceManager &SM,
----------------
kadircet wrote:
> this is fine for the initial version, but we'll likely lose a lot of performance here going forward. we can address it later on though, as this is an internal api and those performance concerns won't be a problem until this is used more widely. 
> 
> we're likely going to call this function multiple times for each symbol, and also despite having different sourcelocations, there'll probably be lots of declarations that're originating from the same FileID.
> Hence we'll want to accumulate outputs on a single container, rather than returning a new container at each call, and also have some sort of file-id based cache.
Acked, agree that this is likely a hot function, we should not be concern about it at the moment.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137320