adamcz updated this revision to Diff 385071.
adamcz added a comment.
addressed review comment
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111318/new/
https://reviews.llvm.org/D111318
Files:
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
clang/include/clang/Sema/Overload.h
clang/lib/Sema/SemaCodeComplete.cpp
clang/lib/Sema/SemaOverload.cpp
Index: clang/lib/Sema/SemaOverload.cpp
===================================================================
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -6456,7 +6456,8 @@
// parameters is viable only if it has an ellipsis in its parameter
// list (8.3.5).
if (TooManyArguments(NumParams, Args.size(), PartialOverloading) &&
- !Proto->isVariadic()) {
+ !Proto->isVariadic() &&
+ shouldEnforceArgLimit(PartialOverloading, Function)) {
Candidate.Viable = false;
Candidate.FailureKind = ovl_fail_too_many_arguments;
return;
@@ -6946,7 +6947,8 @@
// parameters is viable only if it has an ellipsis in its parameter
// list (8.3.5).
if (TooManyArguments(NumParams, Args.size(), PartialOverloading) &&
- !Proto->isVariadic()) {
+ !Proto->isVariadic() &&
+ shouldEnforceArgLimit(PartialOverloading, Method)) {
Candidate.Viable = false;
Candidate.FailureKind = ovl_fail_too_many_arguments;
return;
@@ -15242,3 +15244,21 @@
FunctionDecl *Fn) {
return FixOverloadedFunctionReference(E.get(), Found, Fn);
}
+
+bool clang::shouldEnforceArgLimit(bool PartialOverloading,
+ FunctionDecl *Function) {
+ if (!PartialOverloading || !Function)
+ return true;
+ if (Function->isVariadic())
+ return false;
+ if (const auto *Proto =
+ dyn_cast<FunctionProtoType>(Function->getFunctionType()))
+ if (Proto->isTemplateVariadic())
+ return false;
+ if (auto *Pattern = Function->getTemplateInstantiationPattern())
+ if (const auto *Proto =
+ dyn_cast<FunctionProtoType>(Pattern->getFunctionType()))
+ if (Proto->isTemplateVariadic())
+ return false;
+ return true;
+}
Index: clang/lib/Sema/SemaCodeComplete.cpp
===================================================================
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -5818,7 +5818,8 @@
if (Candidate.Function) {
if (Candidate.Function->isDeleted())
continue;
- if (!Candidate.Function->isVariadic() &&
+ if (shouldEnforceArgLimit(/*PartialOverloading=*/true,
+ Candidate.Function) &&
Candidate.Function->getNumParams() <= ArgSize &&
// Having zero args is annoying, normally we don't surface a function
// with 2 params, if you already have 2 params, because you are
Index: clang/include/clang/Sema/Overload.h
===================================================================
--- clang/include/clang/Sema/Overload.h
+++ clang/include/clang/Sema/Overload.h
@@ -1204,6 +1204,20 @@
return Info;
}
+ // Returns false if signature help is relevant despite number of arguments
+ // exceeding parameters. Specifically, it returns false when
+ // PartialOverloading is true and one of the following:
+ // * Function is variadic
+ // * Function is template variadic
+ // * Function is an instantiation of template variadic function
+ // The last case may seem strange. The idea is that if we added one more
+ // argument, we'd end up with a function similar to Function. Since, in the
+ // context of signature help and/or code completion, we do not know what the
+ // type of the next argument (that the user is typing) will be, this is as
+ // good candidate as we can get, despite the fact that it takes one less
+ // parameter.
+ bool shouldEnforceArgLimit(bool PartialOverloading, FunctionDecl *Function);
+
} // namespace clang
#endif // LLVM_CLANG_SEMA_OVERLOAD_H
Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -2601,6 +2601,104 @@
}
}
+TEST(SignatureHelpTest, Variadic) {
+ const std::string Header = R"cpp(
+ void fun(int x, ...) {}
+ void test() {)cpp";
+ const std::string ExpectedSig = "fun([[int x]], [[...]]) -> void";
+
+ {
+ const auto Result = signatures(Header + "fun(^);}");
+ EXPECT_EQ(0, Result.activeParameter);
+ EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+ }
+ {
+ const auto Result = signatures(Header + "fun(1, ^);}");
+ EXPECT_EQ(1, Result.activeParameter);
+ EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+ }
+ {
+ const auto Result = signatures(Header + "fun(1, 2, ^);}");
+ EXPECT_EQ(1, Result.activeParameter);
+ EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+ }
+}
+
+TEST(SignatureHelpTest, VariadicTemplate) {
+ const std::string Header = R"cpp(
+ template<typename T, typename ...Args>
+ void fun(T t, Args ...args) {}
+ void test() {)cpp";
+ const std::string ExpectedSig = "fun([[T t]], [[Args args...]]) -> void";
+
+ {
+ const auto Result = signatures(Header + "fun(^);}");
+ EXPECT_EQ(0, Result.activeParameter);
+ EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+ }
+ {
+ const auto Result = signatures(Header + "fun(1, ^);}");
+ EXPECT_EQ(1, Result.activeParameter);
+ EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+ }
+ {
+ const auto Result = signatures(Header + "fun(1, 2, ^);}");
+ EXPECT_EQ(1, Result.activeParameter);
+ EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+ }
+}
+
+TEST(SignatureHelpTest, VariadicMethod) {
+ const std::string Header = R"cpp(
+ class C {
+ template<typename T, typename ...Args>
+ void fun(T t, Args ...args) {}
+ };
+ void test() {C c; )cpp";
+ const std::string ExpectedSig = "fun([[T t]], [[Args args...]]) -> void";
+
+ {
+ const auto Result = signatures(Header + "c.fun(^);}");
+ EXPECT_EQ(0, Result.activeParameter);
+ EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+ }
+ {
+ const auto Result = signatures(Header + "c.fun(1, ^);}");
+ EXPECT_EQ(1, Result.activeParameter);
+ EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+ }
+ {
+ const auto Result = signatures(Header + "c.fun(1, 2, ^);}");
+ EXPECT_EQ(1, Result.activeParameter);
+ EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+ }
+}
+
+TEST(SignatureHelpTest, VariadicType) {
+ const std::string Header = R"cpp(
+ void fun(int x, ...) {}
+ auto get_fun() { return fun; }
+ void test() {
+ )cpp";
+ const std::string ExpectedSig = "([[int]], [[...]]) -> void";
+
+ {
+ const auto Result = signatures(Header + "get_fun()(^);}");
+ EXPECT_EQ(0, Result.activeParameter);
+ EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+ }
+ {
+ const auto Result = signatures(Header + "get_fun()(1, ^);}");
+ EXPECT_EQ(1, Result.activeParameter);
+ EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+ }
+ {
+ const auto Result = signatures(Header + "get_fun()(1, 2, ^);}");
+ EXPECT_EQ(1, Result.activeParameter);
+ EXPECT_THAT(Result.signatures, UnorderedElementsAre(Sig(ExpectedSig)));
+ }
+}
+
TEST(CompletionTest, IncludedCompletionKinds) {
Annotations Test(R"cpp(#include "^)cpp");
auto TU = TestTU::withCode(Test.code());
Index: clang-tools-extra/clangd/CodeComplete.cpp
===================================================================
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -908,7 +908,9 @@
SigHelp.activeSignature = 0;
assert(CurrentArg <= (unsigned)std::numeric_limits<int>::max() &&
"too many arguments");
+
SigHelp.activeParameter = static_cast<int>(CurrentArg);
+
for (unsigned I = 0; I < NumCandidates; ++I) {
OverloadCandidate Candidate = Candidates[I];
// We want to avoid showing instantiated signatures, because they may be
@@ -918,6 +920,31 @@
if (auto *Pattern = Func->getTemplateInstantiationPattern())
Candidate = OverloadCandidate(Pattern);
}
+ if (static_cast<int>(I) == SigHelp.activeSignature) {
+ // The activeParameter in LSP relates to the activeSignature. There is
+ // another, per-signature field, but we currently do not use it and not
+ // all clients might support it.
+ // FIXME: Add support for per-signature activeParameter field.
+ //
+ // This only matters for variadic functions/templates, where CurrentArg
+ // might be higher than the number of parameters. When that happens, we
+ // assume the last parameter is variadic and assume all further args are
+ // part of it.
+ int NumParams = 0;
+ if (const auto *F = Candidate.getFunction()) {
+ NumParams = F->getNumParams();
+ if (F->isVariadic())
+ ++NumParams;
+ } else if (auto *T = Candidate.getFunctionType()) {
+ if (auto *Proto = T->getAs<FunctionProtoType>()) {
+ NumParams = Proto->getNumParams();
+ if (Proto->isVariadic())
+ ++NumParams;
+ }
+ }
+ SigHelp.activeParameter =
+ std::min(SigHelp.activeParameter, NumParams - 1);
+ }
const auto *CCS = Candidate.CreateSignatureString(
CurrentArg, S, *Allocator, CCTUInfo, true);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
