kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:430
Located.PreferredDeclaration = bool(Sym.Definition) ? DefLoc : DeclLoc;
- Located.Definition = DefLoc;
+ if (Sym.Definition)
+ Located.Definition = DefLoc;
----------------
this is the 3rd time we are checking for `Sym.Definition`, what about moving
the check on line 410 to down here? Then it would look something like this:
```
LocatedSymbol Located;
Located.PreferredDeclaration = DeclLoc;
if (Sym.Definition) {
auto MaybeDefLoc = indexToLSPLocation(Sym.Definition, MainFilePath);
if (!MaybeDefLoc) {
log("locateSymbolNamedTextuallyAt: {0}", MaybeDefLoc.takeError());
return;
}
Located.PreferredDeclaration = *MaybeDefLoc;
Located.Definition = *MaybeDefLoc;
}
Located.Name = (Sym.Name + Sym.TemplateSpecializationArgs).str();
```
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:285
::testing::Matcher<LocatedSymbol> Sym(std::string Name, Range Decl) {
- return Sym(Name, Decl, llvm::None);
+ return Sym(Name, Decl, Decl);
}
----------------
i am not sure if this change is aligned with the whole idea. we are trying to
make the matcher more explicit, so if user wants to assert on the definition
being equal to some range I think they should be explicitly specifying it. so I
believe this should keep passing `llvm::None` to underlying matcher.
I know it is likely more work than this version, but otherwise we are not
really making it explicit.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84919/new/
https://reviews.llvm.org/D84919
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
