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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for comments! (Still not done, adding tests)



================
Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:67
+      // XXX this is just to make running the tool fast during dev!
+      bool BeginInvocation(CompilerInstance &CI) override {
+        const auto &Inputs = CI.getInvocation().getFrontendOpts().Inputs;
----------------
hokein wrote:
> It is fine for debugging, but I think we don't want this behavior by default.
> 
> global-symbol-builder also supports running a single TU (`global-symbol-builder /path/to/file`), which is sufficient for debugging, I think?
> 
Yeah, I'm not going to check this in, thus the XXX comment :-)

Single TU isn't enough - it doesn't test the reducer. On the other hand the full compilation database is too big. So this option would actually be useful! But it doesn't belong here.


================
Comment at: clangd/index/Index.h:131
   SymbolLocation CanonicalDeclaration;
+  SymbolLocation Definition;
 
----------------
hokein wrote:
> We might want to update the comment above.
I think the comment above is still valid, but added one for definition.


================
Comment at: clangd/index/Merge.cpp:71
+  if (!S.CanonicalDeclaration || R.Definition)
     S.CanonicalDeclaration = R.CanonicalDeclaration;
   if (S.CompletionLabel == "")
----------------
ioeric wrote:
> Do we also need to copy other information such as completion detail, since forward declarations usually have minimum symbol information?
Done. Now if one of the symbols has a definition and the other doesn't, we prefer the one that does. Otherwise we fall back to preferring L over R.

This means we prefer a definition from the global index over a declaration from the local one, which seems OK for now.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42942