[PATCH] D50958: [clangd] Implement findReferences function

[Haojian Wu via Phabricator via cfe-commits]

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