[PATCH] D117037: [clang][CodeComplete] Perform approximate member search in bases

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:3456
 
+TEST(CompletionTest, UndeducedType) {
+  clangd::CodeCompleteOptions Opts;
----------------
"undeducedtype" seems like a strange name for this test - MemberFromBaseOfDependent?


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:3456
 
+TEST(CompletionTest, UndeducedType) {
+  clangd::CodeCompleteOptions Opts;
----------------
sammccall wrote:
> "undeducedtype" seems like a strange name for this test - MemberFromBaseOfDependent?
you might want this test in clang/test/CodeCompletion as well/instead.


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:3459
+  const std::string Code = R"cpp(
+struct Base { Base foo(); };
+
----------------
does this also work if the base is dependent (Base<T>)?


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5414
     if (RD && RD->isCompleteDefinition()) {
-      for (const auto *Member : RD->lookup(CDSME->getMember()))
-        if (const ValueDecl *VD = llvm::dyn_cast<ValueDecl>(Member))
-          return VD->getType().getNonReferenceType();
+      // For c++ records perform a dependent name lookup, which also takes care
+      // of the bases.
----------------
could be a little more precise: "look up member heuristically, including in bases"


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5423
+        }
+      } else {
+        for (const auto *Member : RD->lookup(CDSME->getMember())) {
----------------
This case seems very nearly dead. Technically it is possible to get a CXXDependentScopeMemberExpr in C using recovery expr, but I can't find a case where this is useful. Maybe drop it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117037