sammccall added inline comments.
================
Comment at: clangd/XRefs.cpp:209
+ llvm::DenseSet<SymbolID> QuerySyms;
+ llvm::DenseMap<SymbolID, Location> ResultCandidates;
----------------
the approach could some documentation - I find it hard to follow the code.
This function is probably too big by now and should be split up - but without
understanding the goal I can't suggest how.
================
Comment at: clangd/XRefs.cpp:216
+ log("Could not creat location for Loc");
+ continue;
+ }
----------------
this seems suspicious to me - if we can't find an AST based location for the
symbol, why would that mean giving up without consulting the index?
================
Comment at: clangd/XRefs.cpp:224
+ bool IsDef = GetDefinition(D) == D;
+ // Query the index if there is no definition point in the local AST.
+ if (!IsDef)
----------------
isn't the condition here something else - that there's at least one declaration
that isn't a definition?
================
Comment at: clangd/XRefs.cpp:239
+ assert(it != ResultCandidates.end());
+ if (Sym.Definition) {
+ auto Uri = URI::parse(Sym.Definition.FileURI);
----------------
why are we ignoring the index if it's not a definition (and therefore
preferring the AST one?)
================
Comment at: clangd/XRefs.cpp:261
+
+ for (auto It : ResultCandidates)
+ Result.push_back(It.second);
----------------
Here we're returning exactly one candidate per symbol we identified as a
candidate. There are other choices - why this one?
================
Comment at: clangd/XRefs.h:26
+std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos,
+ const SymbolIndex *const Index =
nullptr);
----------------
nit: drop the second const here? we don't normally bother to mark by-value
parameters as const (particularly not in declarations)
================
Comment at: unittests/clangd/XRefsTests.cpp:109
+TEST(GoToDefinition, WithIndex) {
+ MockFSProvider FS;
----------------
We have a findDefinitionsHelper to avoid all this boilerplate.
Can you extend/overload it it to work with index?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45717
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
