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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Mostly LG, but please take a look at the NITs and the implementation-related comments.



================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:415
 
 Optional<ReferenceLoc> refInDecl(const Decl *D) {
   struct Visitor : ConstDeclVisitor<Visitor> {
----------------
This should return `SmallVector<ReferenceLoc, 2>` now, some declarations can have both decl and non-decl references.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:418
     llvm::Optional<ReferenceLoc> Ref;
-
+    bool DeclRef = true;
     void VisitUsingDirectiveDecl(const UsingDirectiveDecl *D) {
----------------
We don't need this flag, in every case we now whether a reference points to a decl or not.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:439
+
+    void VisitTagDecl(const TagDecl *ND) {
+      Ref =
----------------
Similar to `DeclaratorDecl`, let's handle this in `VisitNamedDecl`


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:444
+
+    void VisitDeclaratorDecl(const DeclaratorDecl *ND) {
+      Ref =
----------------
This should be handled in `VisitNamedDecl`, we should use a helper similar to `getQualifier` from `AST.h` to get the qualifier loc instead.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:464
     llvm::Optional<ReferenceLoc> Ref;
-
+    bool NotDeclRef = false;
     void VisitDeclRefExpr(const DeclRefExpr *E) {
----------------
NIT: don't use negative flags, they are hard to read. Prefer `IsDeclRef`


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:468
+                         E->getNameInfo().getLoc(),
+                         NotDeclRef,
+                         {E->getFoundDecl()}};
----------------
Why do we need this in the first place?
Expressions never declare anything, just pass `false` here


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:495
     llvm::Optional<ReferenceLoc> Ref;
-
+    bool NotDeclRef = false;
     void VisitElaboratedTypeLoc(ElaboratedTypeLoc L) {
----------------
Same here: types don't declare anything normally, remove this field and use `false` directly instead.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:669
                           CCI->getMemberLocation(),
+                          false,
                           {CCI->getAnyMember()}};
----------------
NIT: please add `/*IsDecl=*/` comment here and in other places to make reading the code simpler.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:756
+  if (R.IsDecl)
+    OS << ", decl ref";
   return OS;
----------------
NIT: maybe use `declaration` instead?
`ref` is definitely redundant, we know we're printing a reference.


================
Comment at: clang-tools-extra/clangd/FindTarget.h:90
+  /// True if the reference is a declaration or definition;
+  bool IsDecl;
   // FIXME: add info about template arguments.
----------------
Initialize to make sure we can't ever have UB when using this struct.


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:528
+  /// AST.
+  AllRefs annotateReferencesInMainAST(llvm::StringRef Code) {
+    TestTU TU;
----------------
Why do we need this? Can't we test everything we do by putting into `foo`?

The original idea was to avoid too much output in the tests by letting the setup code live outside the function that's being annotated.
Any reason why we have to do the full file here?

I know it's a weird limitation, but that's one that was already made and I'd keep it this way if it's possible.


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:613
         )cpp",
-           "0: targets = {ns}\n"
-           "1: targets = {alias}\n"},
+           "0: targets = {ns}, decl ref\n"
+           "1: targets = {alias}, decl ref\n"},
----------------
We should keep these non-declaration references.
The actual `using namespace` declaration does not have a name, so we don't report a `declaration` there


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68977