[PATCH] D84919: [clangd] Be more explicit on testing the optional DefLoc in LocatedSymbol.

[Haojian Wu via Phabricator via cfe-commits]

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