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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Added some tests too. More tests for dependent constructs are still missing, but please take a look at what we have for now.



================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:373
+                   DeclRelation::TemplateInstantiation)) &&
+         "namedTarget handles templates on its own");
+  auto Decls = allTargetDecls(N);
----------------
kadircet wrote:
> what is `namedTarget` ?
Used to be called this before. Fixed, thanks.


================
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.
----------------
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.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:381
+      llvm::find_if(Decls, [&](std::pair<const Decl *, DeclRelationSet> D) {
+        if (D.second & ~Mask)
+          return false;
----------------
kadircet wrote:
> why do we discard aliases(specifically underlying decls) ? for example if we have `vector<int>` I believe decl(`template<> std::vector<int>`) will be marked with both TemplateInst and Underlying right ?
For `^vector<int>`, we get `vector<int>(TemplateInstantiation)` and `vector(TemplatePattern)`.
No `Underlying`.

Added a relevant test-case.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:391
+  for (auto &D : Decls) {
+    if (D.second & ~Mask)
+      continue;
----------------
kadircet wrote:
> same here for ignoring underlying decls
As mentioned in the previous comment, we want to discard underlying. Underlying decl was not written in the source code, therefore should not be returned by this function.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:399
+Optional<ReferenceLoc> refInDecl(const Decl *D) {
+  struct Visitor : ConstDeclVisitor<Visitor> {
+    llvm::Optional<ReferenceLoc> Ref;
----------------
kadircet wrote:
> these three(usingdirective,using,namespacealias) decls are the only ones that can have a nestednamespecifier.
> 
> Are we sure these are the only ones that can reference a decl? For example what about a `ConstructorDecl` with initializers referencing `FieldDecl`s, I believe this function should rather return multiple locations in such cases.
This function is non-recursive and `ConstructorInitializer` is a separate node under `ConsturctorDecl`.
Any reference to fields are owned (and reported when `ConstructorInitializer` is visited)

There might be more decls that I missed here, though, hopefully we'll find and add them later.

It's a bit hard to get all cases on the first try.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:409
+    void VisitUsingDecl(const UsingDecl *D) {
+      Ref = ReferenceLoc{D->getQualifierLoc(), D->getUsingLoc(),
+                         explicitReferenceTargets(DynTypedNode::create(*D),
----------------
kadircet wrote:
> I think `getUsingLoc` will return the location for `using` keyword, not the name location. Maybe use `getLocation` ?
Thanks for spotting this! This was supposed to be a temporary placeholder until I figure out a proper location.
But I forgot about it.

Fixed and added a test.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:426
+
+Optional<ReferenceLoc> refInExpr(const Expr *E) {
+  struct Visitor : ConstStmtVisitor<Visitor> {
----------------
kadircet wrote:
> again I believe this list is non-exhaustive, e.g. designatedinitilaizers. and again this also needs to return a vector instead.
Added a FIXME to add more exhaustive handling.
I'll make sure to fix it soon, but would want to first land non-exhaustive version.

I would try avoiding returning more than one element here. For designated initializers we seem to have multiple designators. Since this function is non-recursive, it should not return those.

Instead we would require our clients (i.e. our visitor) to call a separate function for each of the designators.


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


================
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:
> 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.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:570
+    visitNode(DynTypedNode::create(L));
+    // Inner type is missing information about its qualifier, skip it.
+    if (auto TL = L.getTypeLoc())
----------------
kadircet wrote:
> why not just visitNode(L) and traverse(prefix(L)) instead of calling base?
For code reuse. See other comment too


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:581
+      return;
+    // FIXME: this should be done by nodeReference.
+    if (Ref->NameLoc.isInvalid() || Ref->NameLoc.isMacroID())
----------------
kadircet wrote:
> what is `nodeReference`?
Now renamed to `explicitReference`. Thanks.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:594
+
+void findExplicitReferences(Stmt *S,
+                            llvm::function_ref<void(ReferenceLoc)> Out) {
----------------
kadircet wrote:
> So this endpoint doesn't seem to have downsides above(not visiting all referenced decls) as it performs traversal through an extra ASTVisitor.
> 
> What is the point of exposing the first method? Maybe we can getaway with not exposing it or strictly mentioning it might not return references in the children nodes?
I think we'll need it for D56610 (qualify name under cursor).
But happy to remove this from public API for now until we have a use-case for it.

Also updated a comment to explicitly mention it does not recurse into child nodes, hope its intention is clear now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67826