[PATCH] D138200: [include-cleaner] Make use of locateSymbol in WalkUsed

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

sorry for the delay, just realize it is sitting in the review list, left some comments.



================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:25
+// Gets all the providers for a symbol by tarversing each location.
+llvm::SmallVector<Header> findAllHeaders(const Symbol &S,
+                                         const SourceManager &SM,
----------------
nit: this name is similar to `findHeaders` (not sure this is intended), I'd rather use a different name (something like `headersForSymbol`)


================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:44
+
+  // Cache for decl to header mappings, as the same decl might be referenced in
+  // multiple locations and finding providers for each location is expensive.
----------------
I agree the place here might be on a performance-critical path, but I think the idea of using cache probably needs some bits of design, the current implementation seems half baked (there is also a FIXME in the findAllHeaders saying we should implement another cache for it, so it is unclear to me what's the whole picture).

Can we just leave out all cache in this patch with a FIXME? And having a follow-up patch for that?


================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:46
+  // multiple locations and finding providers for each location is expensive.
+  std::unordered_map<NamedDecl *, llvm::SmallVector<Header>> DeclToHeaders;
   tooling::stdlib::Recognizer Recognizer;
----------------
any reason not using `llvm::DenseMap`?


================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:52
       SymbolReference SymRef{Loc, ND, RT};
-      if (auto SS = Recognizer(&ND)) {
-        // FIXME: Also report forward decls from main-file, so that the caller
-        // can decide to insert/ignore a header.
-        return CB(SymRef, findHeaders(*SS, SM, PI));
-      }
-      // FIXME: Extract locations from redecls.
-      return CB(SymRef, findHeaders(ND.getLocation(), SM, PI));
+      auto Inserted = DeclToHeaders.try_emplace(&ND);
+      if (Inserted.second)
----------------
nit: consider using structure binding to get rid of the `Insert.first`, `Insert.second` magic names.


================
Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:141
+
 TEST(WalkUsed, MacroRefs) {
   llvm::Annotations Hdr(R"cpp(
----------------
it feels weird that all other tests are using WalkUsedTest, but not this one.

The logic are mostly the same except that this test doesn't need to build the AST nodes, if we change  `offsetToProviders` method to `offset(llvm::ArrayRef<Decl*> TopLevelDecl, llvm::ArrayRef<SymbolReference> MacroRef, SourceManager &SM)`, I think we can use it in this test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138200