[PATCH] D68977: [clangd] Report declaration references in findExplicitReferences.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Mostly NITs from my side, the change LG, thanks!



================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:422
+      // "using namespace" declaration doesn't have a name.
+      Refs.push_back({D->getQualifierLoc(),
+                      D->getIdentLocation(),
----------------
Same comment as below: could you say `ReferenceLoc` explicitly here and in other places? 
Plain initializer lists are somewhat hard to parse


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:436
+      // For namespace alias, "namespace Foo = Target;", we add two references.
+      VisitNamedDecl(D); // add a declaration reference for Foo.
+      // Add a non-declaration reference for Target.
----------------
NIT: maybe put this comment on a previous line?
Should read more naturally.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:658
+    } else if (auto *E = N.get<Expr>()) {
+      if (auto Ref = refInExpr(E))
+        return {*Ref};
----------------
I'd probably suggest to return `SmallVector<ReferenceLoc, 2>` from all functions, so that they compose better in this situation.

Although `Optional<ReferenceLoc>` captures the semantics of some of those better, I don't think it's particularly useful at this point.
And having to deconstruct all optionals here leads to a lot of clunky code, it'd be much better if we could keep the structure the same as it's used to be.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:661
+    } else if (auto *NNSL = N.get<NestedNameSpecifierLoc>()) {
+      return {{NNSL->getPrefix(), NNSL->getLocalBeginLoc(), false,
+               explicitReferenceTargets(
----------------
Could we have `{ReferenceLoc{` instead of `{{`?
Plain initializer lists are hard to comprehend, especially when they're nested.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:667
+        return {*Ref};
+      // return refInTypeLoc(*TL);
+    } else if (const CXXCtorInitializer *CCI = N.get<CXXCtorInitializer>()) {
----------------
Remove this comment? Seems to be a leftover from experiments


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:670
       if (CCI->isBaseInitializer())
-        return refInTypeLoc(CCI->getBaseClassLoc());
+        if (auto Ref = refInTypeLoc(CCI->getBaseClassLoc()))
+          return {*Ref};
----------------
NIT: add braces around an inner multi-line if?


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:2280
     }
-    EXPECT_THAT(Names, UnorderedElementsAreArray(C.ExpectedDecls));
+    EXPECT_THAT(Names, UnorderedElementsAreArray(C.ExpectedDecls))
+        << File.code();
----------------
This looks unrelated. Maybe revert and land separately?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68977/new/

https://reviews.llvm.org/D68977