[PATCH] D67826: [clangd] A helper to find explicit references and their names

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked an inline comment as done.
kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:376
+
+  // We prefer to return template instantiation, but fallback to template
+  // pattern if instantiation is not available.
----------------
ilya-biryukov wrote:
> kadircet wrote:
> > can we simplify this by populating two vectors, Instantiations and Patterns, and then returning the patterns iff instantiations are empty?
> Did something very similar. PTAL.
LG, thanks!


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:432
+      Ref = ReferenceLoc{
+          E->getQualifierLoc(), E->getNameInfo().getLoc(), {E->getDecl()}};
+    }
----------------
ilya-biryukov wrote:
> kadircet wrote:
> > I believe it is better to return `getFoundDecl` then `getDecl`, the former respects using declarations.
> Good point. Done. Added a test too.
I was actually referring to `DeclRefExpr`, change seems to be on `MemberExpr`


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:548
+  bool TraverseElaboratedTypeLoc(ElaboratedTypeLoc L) {
+    // ElaboratedTypeLoc will reports information for its inner type loc.
+    // Otherwise we loose information about inner types loc's qualifier.
----------------
ilya-biryukov wrote:
> kadircet wrote:
> > why not just traversenestednamespecifier and `visitNode(L)` instead of calling the base::traverse ?
> To avoid re-implementing the traversal logic. `Base::Traverse` does exactly that, we shouldn't re-implement traversal ourselves.
I see but that should help get rid of `TypeLocsToSkip` and some extra traversals. I believe it would be worth getting rid of the additional state management, but up to you.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:370
+namespace {
+/// Find a declaration explicitly referenced in the source code defined by \p N.
+/// For templates, will prefer to return a template instantiation whenever
----------------
nit: s/a declaration/declaration(s)/


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:519
+
+namespace {
+
----------------
unnecessary closing and opening of anon namespace


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:577
+  /// (!) For the purposes of this function declarations are not considered to
+  ///     be references. However, declarations can have:wa references inside
+  ///     them, e.g. 'namespace foo = std' references namespace 'std' and this
----------------
`:wa` is still around `have:wa references` :D


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:609
+    // FIXME: should this be done by 'explicitReference'?
+    if (Ref->NameLoc.isInvalid() || Ref->NameLoc.isMacroID())
+      return;
----------------
I can see the second check is for getting rid of references coming from macro expansions. But what exactly is the first one for? How can we get an invalid nameloc, could you also add it as a comment?


================
Comment at: clang-tools-extra/clangd/FindTarget.h:96
+};
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, ReferenceLoc R);
+
----------------
Is this for testing purposes? maybe move it into `findtargettests.cpp` or make it a member helper like `Print(SourceManager&)` so that it can also print locations etc.?


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:498
+    std::string AnnotatedCode;
+    unsigned NextOffset = 0;
+    for (unsigned I = 0; I < Refs.size(); ++I) {
----------------
this sounds more like `LastOffset` or `PrevOffset`


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:504
+      assert(Pos.isValid());
+      if (Pos.isMacroID()) // FIXME: figure out how to show macro locations.
+        Pos = SM.getExpansionLoc(Pos);
----------------
I believe these are already filtered-out by `findExplicitReferences` ?


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:600
+           "5: targets = {a::b::S}\n"
+           "6: targets = {a::b::S::type}, qualifier = 'struct S::'\n"},
+          // Simple templates.
----------------
Is it OK for this case to be different than `X::a` above?

also shouldn't this be `a::b::struct S` or `None` ?(my preference would be towards None)


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:638
+           "0: targets = {y}\n"
+           "1: targets = {Y::foo}\n"},
+      };
----------------
again qualifiers seems to be inconsistent with the rest of the cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67826