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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked 9 inline comments as done.
ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:432
+      Ref = ReferenceLoc{
+          E->getQualifierLoc(), E->getNameInfo().getLoc(), {E->getDecl()}};
+    }
----------------
kadircet wrote:
> 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`
Good point. Fixed that one too, thanks!


================
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.
----------------
kadircet wrote:
> 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.
I like the idea and actually tried to implement it before, but failed. The nested-name-specifier case is simple and we could actually implement it.

However, the `ElaboratedTypeLoc` case is more complicated: we have to call `Base::TraverseTypeLoc` to make sure we recursively visit children of `ElaboratedTypeLoc::getNamedType` (e.g. template arguments inside the typeloc). However, `Base::TraverseTypeLoc` automatically calls `VisitTypeLoc` and our qualifier is forgotten again.

Let me know if there's a way to implement this that I missed, though.


================
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
----------------
kadircet wrote:
> `:wa` is still around `have:wa references` :D
Thanks. This slipped my attention last time :-)


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:609
+    // FIXME: should this be done by 'explicitReference'?
+    if (Ref->NameLoc.isInvalid() || Ref->NameLoc.isMacroID())
+      return;
----------------
kadircet wrote:
> 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?
Ah, thanks for spotting this. I don't think we want to filter out macro references, fixed that and added a test

The first one is a precaution against various implicit nodes getting into the traversal. Normally this shouldn't happen, unless:
- RecursiveASTVisitor has bugs and visits implicit nodes (e.g. generated begin() calls for range-based-for )
- Users start traversal from an implicitly-generated statement.

I couldn't come up with examples of that actually happening, but I'm also scared of adding an assertion as I feel this would inevitably fail at some point.
At the same time, the whole point of this function is to only expose those references that could be meaningfully found in the source code (e.g. for refactoring and syntax highlighting), i.e. they should always have (at least) source location of the name.

Added a comment, let me know if this could be made clearer, though.


================
Comment at: clang-tools-extra/clangd/FindTarget.h:96
+};
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, ReferenceLoc R);
+
----------------
kadircet wrote:
> 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.?
Yes, this is for output in the test. Moving to `findTargetTests.cpp`, we risk ODR violations in the future.
In any case, it's useful to have debug output and `operator<<` is common for that purpose: it enables `llvm::toString` and, consecutively, `llvm::formatv("{0}", R)`.

What are the downsides of having it?



================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:498
+    std::string AnnotatedCode;
+    unsigned NextOffset = 0;
+    for (unsigned I = 0; I < Refs.size(); ++I) {
----------------
kadircet wrote:
> this sounds more like `LastOffset` or `PrevOffset`
That's the next character in `Code` we need to process, renamed to `NextCodeChar` to specifically mention what string we're referring to.


================
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);
----------------
kadircet wrote:
> I believe these are already filtered-out by `findExplicitReferences` ?
Not anymore (wasn't supposed to in the first place). Added tests too.
See the other comment for details.


================
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.
----------------
kadircet wrote:
> 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)
Qualifier captures exactly what's written in the source code, so it seems correct: `S::` is what written down.
`struct` comes from the printing function in clang and I don't think it's worth changing. I believe this comes from the fact that qualifier is a type in this case.

Why would the qualifier be `None`? Maybe the purpose of this field is unclear?


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:638
+           "0: targets = {y}\n"
+           "1: targets = {Y::foo}\n"},
+      };
----------------
kadircet wrote:
> again qualifiers seems to be inconsistent with the rest of the cases.
There are no qualifiers written in the source code in any of the two references, therefore both qualifiers are empty.
Again, please let me know if the purpose of `Qualifier` is unclear. Happy to comment or change to make its purpose clear.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67826