hokein 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;
----------------
kadircet wrote:
> 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();
> ```
good point.
================
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);
}
----------------
kadircet wrote:
> 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.
yeah, it seems odd. I think this function is used as a syntax sugar to check
`LocatedSymbol` has the *same* decl/def range, most callers have this
assumptions.
options:
1. keeps it as is, and rename it to something like `SymWithSameDeclDefRange` (a
better name is welcome);
2. remove it, and use the 3-arg matcher above, callers have to be more verbose
(passing decl/def ranges explicitly, even they are the same), and we have 15
references;
1 is my preference, what do you think?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84919/new/
https://reviews.llvm.org/D84919
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
