[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Thanks for the patch, these are nice improvements



================
Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:258
     case CodeCompletionString::CK_RightParen:
+      if (DropFunctionArguments &&
+          ResultKind == CodeCompletionResult::RK_Declaration)
----------------
It looks like we are assuming two things:

 1. Any LeftParen in an RK_Declaration starts a function argument list
 2. Everything that comes after the function argument list can be discarded

I think these assumptions are fine to make, but let's be explicit about them in a comment


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:536
+        template <typename T, int U>
+        void generic(T);
+        template <typename T, int U = 3>
----------------
Suggestion: it would be slightly more interesting to make the function signature `generic(U)`. Then, the function can be called as `generic<T>(u)` (with the template parameter `U` deduced), and the [LastDeducibleArgument](https://searchfox.org/llvm/rev/bd7c6e3c48e9281ceeaae3a93cc493b35a3c9f29/clang/lib/Sema/SemaCodeComplete.cpp#3553) logic should make sure that only `<T>` is added to the code completion string, not `<T, U>`.


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:580
     auto Results = completions(TU, P, /*IndexSymbols*/ {}, Opts);
     EXPECT_THAT(Results.Completions,
                 Contains(AllOf(named("method"), snippetSuffix(""))));
----------------
Since you're updating this test anyways, could you add `signature()` matchers for the non-template cases as well please?


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp:178
+      /*DropFunctionArguments=*/true);
+  EXPECT_EQ(Signature, "<typename T, int U>(arg1, arg2)");
+  EXPECT_EQ(Snippet, "<${1:typename T}, ${2:int U}>");
----------------
Maybe add:

```
// Arguments dropped from snippet, kept in signature
```

so someone reading the test knows it's deliberate


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:345
   void AddResult(Result R, DeclContext *CurContext, NamedDecl *Hiding,
-                 bool InBaseClass);
+                 bool InBaseClass, QualType BaseType);
 
----------------
The word "base" is a bit overloaded in this signature; in the parameter `InBaseClass` it refers to inheritance, but in `BaseType` it refers to the base expression of a `MemberExpr`.

Maybe we can name the new parameter `BaseExprType` to avoid confusion?


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1285
     Result.ShadowDecl = Using;
     AddResult(Result, CurContext, Hiding);
     return;
----------------
Should we propagate `BaseType` into this recursive call?


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1425
+        R.FunctionCanBeCall =
+            MaybeDerived == MaybeBase || MaybeDerived->isDerivedFrom(MaybeBase);
+      }
----------------
Is there any situation where `MaybeDerived == MaybeBase || MaybeDerived->isDerivedFrom(MaybeBase)` is false?

I am wondering if we can simplify this to:

```
if (!BaseType.isNull()) {
  R.FunctionCanBeCall = true;
}
```


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3577
       // containing all of the arguments up to the first deducible argument.
+      // Or, if this isn't a call, emit all the template arguments
+      // to disambiguate the (potential) overloads.
----------------
1. If this is not a call, we can avoid running the `Sema::MarkDeducedTemplateParameters` operation altogether.

2. A future improvement we could consider: if this is not a call, try to detect cases where the template parameters can be deduced from the surrounding context (["Deducing template arguments taking the address of a function template "](https://eel.is/c++draft/temp.deduct.funcaddr)). Maybe add a FIXME for this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156605