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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
