hokein added inline comments.
================
Comment at: clangd/XRefs.cpp:666
+std::vector<Location> references(ParsedAST &AST, Position Pos,
+ bool IncludeDeclaration,
+ const SymbolIndex *Index) {
----------------
sammccall wrote:
> I'm not sure unpacking the reference options into individual bools is going
> to scale well. I'd suggest passing the whole struct instead.
Removed this parameter.
================
Comment at: clangd/XRefs.cpp:681
+ : "Non-local\n");
+ if (!clang::index::isFunctionLocalSymbol(D))
+ NonLocalIDs.insert(*ID);
----------------
sammccall wrote:
> (This saves us hitting the index to look up references for one symbol, not
> sure if it's worth it at all).
>
> I wouldn't particularly trust the helpers in clang::index::. Indeed the
> implementation looks wrong here (e.g. it would treat lambda captures as
> global, I think?)
>
> I'd prefer D->getParentFunctionOrMethod() here.
>
>
I think it is worth the effort here. At least for function-local symbols,
traversing AST is sufficient to get complete xref results.
================
Comment at: clangd/XRefs.cpp:688
+ SymbolOccurrenceKind::Reference | SymbolOccurrenceKind::Definition;
+ if (IncludeDeclaration)
+ Filter |= SymbolOccurrenceKind::Declaration;
----------------
sammccall wrote:
> I'm not sure this is the best interpretation of LSP `includeDeclaration`.
>
> The LSP spec is very vague here, and we can usually assume it's written with
> typescript in mind :-) where the declaration/definition distinction doesn't
> really exist.
> It appears the distinction they're trying to draw is declaration vs "only"
> reference, rather than definition vs "only" declaration. So I think if we're
> going to respect this flag, we should *exclude* definitions when the flag is
> false.
>
> Alternatively we could punt on this and just ignore the flag for now, and add
> it in a followup.
Ignore this flag, and always return all kinds.
================
Comment at: clangd/XRefs.cpp:694
+
+ SymbolCollector Collector({nullptr, &Opts}, {});
+ index::IndexingOptions IndexOpts;
----------------
sammccall wrote:
> Reusing symbolcollector here seems odd.
>
> It forces us to go through SymbolID rather than just using the Decl*. This is
> certainly reliable for global symbols, but I've got no idea how robust USRs
> are for local symbols. It also ends up building the Symbol objects, which we
> then throw away.
>
> How much of SymbolCollector are we really reusing, vs a custom
> IndexDataConsumer? How is this different from document-highlights? Maybe we
> can reuse the consumer.
I refined the highlight IndexDataConsumer, and now the common code is shared.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50958
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
