[PATCH] D42942: [clangd] Collect definitions when indexing.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for the comments!
And sorry for the delay, I was haunted by useless gtest messages, which https://reviews.llvm.org/D43091 should fix.



================
Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:159
+  // Output phase: emit YAML for result symbols.
   for (const auto &Sym : UniqueSymbols)
+    llvm::outs() << SymbolToYAML(Sym);
----------------
hokein wrote:
> nit: we could get rid of the loop by using `SymbolsToYAML(UniqueSymbols)` .
Yeah, but we'd put the whole text form of the index into a single in-memory string, which seems pretty wasteful. Building a YAML output for each seems silly too though.

Changed `SymbolsToYAML` to take a `raw_ostream` ref parameter to write to.


================
Comment at: clangd/index/Merge.cpp:93
     S.Detail = Scratch;
   } else if (L.Detail)
+    /* Current state: S.Detail = O.Detail */;
----------------
ioeric wrote:
> `else if (S.Detail)`?
> 
> `/*Current state: S.Detail = S.Detail*/`?
Good catch.
Reworked the structure here to avoid needing to name this case, since it was awkward.


================
Comment at: clangd/index/SymbolCollector.cpp:210
+      BasicSymbol = addDeclaration(*ND, std::move(ID));
+    if (Roles & static_cast<unsigned>(index::SymbolRole::Definition))
+      addDefinition(*cast<NamedDecl>(ASTNode.OrigD), *BasicSymbol);
----------------
hokein wrote:
> ioeric wrote:
> > It seems that we store definition even if it's the same as declaration, which might be true for most classes? This is probably fine, but it would be worth documenting. Or maybe consider not storing the same location twice?
> I think it is fine to store the same location twice. We can't tell whether the CanonicalLoc is a definition or not.
Documented that these may be the same.
We wouldn't actually save any memory by avoiding saving this twice - the filename is a stringref to the same data, and the offsets get stored even if they're null.


================
Comment at: clangd/index/SymbolCollector.cpp:236
+  std::string FileURI;
+  // FIXME: we may want a different "canonical" heuristic than clang chooses.
+  if (auto DeclLoc = getSymbolLocation(ND, SM, Opts, FileURI))
----------------
ioeric wrote:
> Could you elaborate a bit more on this one? What is the current heuristic, and what would we prefer?
Done a bit, though I don't know the detailed answer to either question.
Using forward declarations of classes as canonical indicates that we probably want something better.


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:108
+                                     "-std=c++11",
+                                     "-include",
+                                     llvm::sys::path::filename(TestHeaderName),
----------------
ioeric wrote:
> neat!
Indeed :-)
This is needed because otherwise all the offsets in the annotated testcase are off.
I got it slightly wrong though: need the full path to the header name because it's not resolved relative to the main file.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42942