[PATCH] D42942: [clangd] Collect definitions when indexing.
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 6 06:21:02 PST 2018
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
More information about the cfe-commits
mailing list