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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
