[clangd] Handle the missing constructor initializers in findExplicitReferences.

[ilya-biryukov]

LGTM, thanks!

clang-tools-extra/clangd/FindTarget.cpp

646

This looks misindented. Could you clang-format?

[ilya-biryukov]

clang-tools-extra/clangd/unittests/FindTargetTests.cpp

844

Could you also add a test with the initializer containing a base class? It produces a different kind of CXXConstructorInitializer

[merge_guards_bot]

Build result: fail - 33623 tests passed, 1 failed and 462 were skipped.

failed: LLVM.tools/llvm-ar/mri-utf8.test

Log files: cmake-log.txt, ninja_check_all-log.txt, CMakeCache.txt

[ilya-biryukov]

NIT: there's a typo in the revision title: should be constructor, not consturctor

[hokein]

• fix some issues in the existing implementation;
• add tests for base/delegating initializer;

[hokein]

In D69241#1716787, @ilya-biryukov wrote:

NIT: there's a typo in the revision title: should be constructor, not consturctor

Done.

clang-tools-extra/clangd/unittests/FindTargetTests.cpp

844

Done.

While adding more tests, I discover some issues in the implementation, also fixed them in this patch, please review the patch again.

[merge_guards_bot]

Build result: fail - 59521 tests passed, 1 failed and 805 were skipped.

failed: LLVM.tools/llvm-ar/mri-utf8.test

Log files: cmake-log.txt, ninja_check_all-log.txt, CMakeCache.txt

[ilya-biryukov]

clang-tools-extra/clangd/FindTarget.cpp

677–684

I believe base initializers are the only ones left, but the comment suggests there are more cases. Maybe change to:

// Base initializers are handled by visting the type loc.

Or am I missing something?

[hokein]

clang-tools-extra/clangd/FindTarget.cpp

677–684

there are others, delegating initializers.

[ilya-biryukov]

clang-tools-extra/clangd/FindTarget.cpp

677–684

Ah, interesting... Could we add them to the tests too?

[hokein]

clang-tools-extra/clangd/FindTarget.cpp

677–684

it has been added in the tests already, without the change here, the test will trigger the assert(CCI->isAnyMemberInitializer());.

// delegating initializer
class $10^Foo {
  $11^Foo(int$12^);
  $13^Foo(): $14^Foo(111) {}
};

[merge_guards_bot]

Build result: fail - 59521 tests passed, 1 failed and 805 were skipped.

failed: LLVM.tools/llvm-ar/mri-utf8.test

Log files: cmake-log.txt, ninja_check_all-log.txt, CMakeCache.txt

clang-tools-extra/clangd/FindTarget.cpp

if (!L.getNestedNameSpecifier())
       return true;
     visitNode(DynTypedNode::create(L));
     // Inner type is missing information about its qualifier, skip it.
     if (auto TL = L.getTypeLoc())
       TypeLocsToSkip.insert(TL.getBeginLoc().getRawEncoding());
     return RecursiveASTVisitor::TraverseNestedNameSpecifierLoc(L);
   }
 
+  bool TraverseConstructorInitializer(CXXCtorInitializer *Init) {
+    visitNode(DynTypedNode::create(*Init));
+    return RecursiveASTVisitor::TraverseConstructorInitializer(Init);
+  }
+
 private:
   /// Obtain information about a reference directly defined in \p N. Does not
   /// recurse into child nodes, e.g. do not expect references for constructor
   /// initializers
   ///
   /// Any of the fields in the returned structure can be empty, but not all of
   /// them, e.g.
   ///   - for implicitly generated nodes (e.g. MemberExpr from range-based-for),
if (auto *E = N.get<Expr>())
       return refInExpr(E);
     if (auto *NNSL = N.get<NestedNameSpecifierLoc>())
       return {ReferenceLoc{NNSL->getPrefix(), NNSL->getLocalBeginLoc(), false,
                            explicitReferenceTargets(DynTypedNode::create(
                                *NNSL->getNestedNameSpecifier()))}};
     if (const TypeLoc *TL = N.get<TypeLoc>())
       return refInTypeLoc(*TL);
     if (const CXXCtorInitializer *CCI = N.get<CXXCtorInitializer>()) {
-      if (CCI->isBaseInitializer())
-        return refInTypeLoc(CCI->getBaseClassLoc());
-      assert(CCI->isAnyMemberInitializer());
+      // Other type initializers (e.g. base initializer) are handled by visiting
+      // the typeLoc.
+      if (CCI->isAnyMemberInitializer()) {
         return {ReferenceLoc{NestedNameSpecifierLoc(),
                              CCI->getMemberLocation(),
                              /*IsDecl=*/false,
                              {CCI->getAnyMember()}}};
       }
+    }
     // We do not have location information for other nodes (QualType, etc)
     return {};
   }
 
   void visitNode(DynTypedNode N) {
     for (const auto &R : explicitReference(N))
       reportReference(R, N);
   }

clang-tools-extra/clangd/unittests/FindTargetTests.cpp

std::pair</*Code*/ llvm::StringRef, /*References*/ llvm::StringRef> Cases[] =
               $0^wrapper<$1^func> $2^w;
               $3^FuncParam();
             }
         )cpp",
            "0: targets = {wrapper<&func>}\n"
            "1: targets = {func}\n"
            "2: targets = {w}, decl\n"
            "3: targets = {FuncParam}\n"},
+          // declaration references.
           {R"cpp(
              namespace ns {}
              class S {};
              void foo() {
                class $0^Foo { $1^Foo(); ~$2^Foo(); int $3^field; };
                int $4^Var;
                enum $5^E { $6^ABC };
                typedef int $7^INT;
                using $8^INT2 = int;
                namespace $9^NS = $10^ns;
              }
            )cpp",
            "0: targets = {Foo}, decl\n"
            "1: targets = {foo()::Foo::Foo}, decl\n"
            "2: targets = {Foo}\n"
            "3: targets = {foo()::Foo::field}, decl\n"
            "4: targets = {Var}, decl\n"
            "5: targets = {E}, decl\n"
            "6: targets = {foo()::ABC}, decl\n"
            "7: targets = {INT}, decl\n"
            "8: targets = {INT2}, decl\n"
            "9: targets = {NS}, decl\n"
            "10: targets = {ns}\n"},
+          // cxx constructor initializer.
+          {R"cpp(
+             class Base {};
+             void foo() {
+               // member initializer
+               class $0^X {
+                 int $1^abc;
+                 $2^X(): $3^abc() {}
+               };
+               // base initializer
+               class $4^Derived : public $5^Base {
+                 $6^Base $7^B;
+                 $8^Derived() : $9^Base() {}
+               };
+               // delegating initializer
+               class $10^Foo {
+                 $11^Foo(int$12^);
+                 $13^Foo(): $14^Foo(111) {}
+               };
+             }
+           )cpp",
+           "0: targets = {X}, decl\n"
+           "1: targets = {foo()::X::abc}, decl\n"
+           "2: targets = {foo()::X::X}, decl\n"
+           "3: targets = {foo()::X::abc}\n"
+           "4: targets = {Derived}, decl\n"
+           "5: targets = {Base}\n"
+           "6: targets = {Base}\n"
+           "7: targets = {foo()::Derived::B}, decl\n"
+           "8: targets = {foo()::Derived::Derived}, decl\n"
+           "9: targets = {Base}\n"
+           "10: targets = {Foo}, decl\n"
+           "11: targets = {foo()::Foo::Foo}, decl\n"
+           // FIXME: we should exclude the built-in type.
+           "12: targets = {}, decl\n"
+           "13: targets = {foo()::Foo::Foo}, decl\n"
+           "14: targets = {Foo}\n"},
+
       };
 
   for (const auto &C : Cases) {
     llvm::StringRef ExpectedCode = C.first;
     llvm::StringRef ExpectedRefs = C.second;
 
     auto Actual =
         annotateReferencesInFoo(llvm::Annotations(ExpectedCode).code());
     EXPECT_EQ(ExpectedCode, Actual.AnnotatedCode);
     EXPECT_EQ(ExpectedRefs, Actual.DumpedReferences) << ExpectedCode;
   }
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang