[PATCH] D50385: [clangd] Collect symbol occurrences in SymbolCollector

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This looks pretty good!



================
Comment at: clangd/index/Index.h:376
+
+   llvm::ArrayRef<SymbolOccurrence> find(const SymbolID &ID) const {
+     auto It = Occurrences.find(ID);
----------------
assert frozen?
looking up in a non-frozen array is probably a mistake.
if we choose to optimize this, it probably won't be possible.


================
Comment at: clangd/index/Index.h:377
+   llvm::ArrayRef<SymbolOccurrence> find(const SymbolID &ID) const {
+     auto It = Occurrences.find(ID);
+     if (It == Occurrences.end())
----------------
return Occurrences.lookup(ID)?


================
Comment at: clangd/index/SymbolCollector.cpp:227
 
+SymbolOccurrenceKind ToOccurrenceKind(index::SymbolRoleSet Roles) {
+  SymbolOccurrenceKind Kind;
----------------
nit: toOccurrenceKind


================
Comment at: clangd/index/SymbolCollector.cpp:229
+  SymbolOccurrenceKind Kind;
+  for (auto Mask :
+       {SymbolOccurrenceKind::Declaration, SymbolOccurrenceKind::Definition,
----------------
If you want to filter out the unsupported bits, maybe just add an explicit `AllOccurrenceKinds` constant to the header file, and `return AllOccurrenceKinds & Roles` here? (plus casts)


================
Comment at: clangd/index/SymbolCollector.cpp:328
+  if ((static_cast<unsigned>(Opts.OccurrenceFilter) & Roles) &&
+      SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
+    DeclOccurrences[ND].emplace_back(Loc, Roles);
----------------
just compute the spelling loc once and reuse?


================
Comment at: clangd/index/SymbolCollector.cpp:329
+      SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
+    DeclOccurrences[ND].emplace_back(Loc, Roles);
+
----------------
you get the spelling loc on the previous line to check for mainfile - so surely we should be using spelling loc here?


================
Comment at: clangd/index/SymbolCollector.cpp:455
+
+  for (auto& It : DeclOccurrences) {
+    if (auto ID = getSymbolID(It.first)) {
----------------
nit: const auto& for clarity since we're not mutating


================
Comment at: clangd/index/SymbolCollector.cpp:460
+          std::string FileURI;
+          if (auto DeclLoc = getTokenLocation(LocAndRole.first,
+                                              ASTCtx->getSourceManager(), Opts,
----------------
so this seems maybe  gratuitously inefficient, we're copying the filename then going through the URI conversion dance for each reference - even though the filename is the same for each.

consider splitting out part of `getTokenLocation` into `getTokenRange(SymbolLocation&)` and only calling that here.


================
Comment at: clangd/index/SymbolCollector.h:54
     // Populate the Symbol.References field.
     bool CountReferences = false;
     // Every symbol collected will be stamped with this origin.
----------------
this should be next to OccurrenceFilter, they're very closely related (the name mismatch is a little unfortunate)


================
Comment at: clangd/index/SymbolCollector.h:121
+  using DeclOccurrence = std::pair<SourceLocation, index::SymbolRoleSet>;
+  llvm::DenseMap<const NamedDecl*, std::vector<DeclOccurrence>> DeclOccurrences;
+  // All symbol occurrences collected from the AST, assembled on finish().
----------------
please move next to ReferencedDecls/ReferencedMacros so the comment applies to this too


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:466
+      SymbolOccurrences.find(findSymbol(Symbols, "Foo").ID),
+      testing::UnorderedPointwise(OccurrenceRange(), Main.ranges("foo")));
+  EXPECT_THAT(
----------------
this is cute - if possible, consider adding a matcher factory function for readability here, so you can write `EXPECT_THAT(..., HaveRanges(Main.ranges("foo"))`


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50385