[PATCH] D50555: [clangd] Introduce scoring mechanism for SignatureInformations.
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE339547: [clangd] Introduce scoring mechanism for
SignatureInformations. (authored by kadircet, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D50555?vs=160306=160307#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50555
Files:
clangd/CodeComplete.cpp
clangd/Quality.cpp
clangd/Quality.h
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1515,6 +1515,28 @@
}
}
+TEST(SignatureHelpTest, OverloadsOrdering) {
+ const auto Results = signatures(R"cpp(
+void foo(int x);
+void foo(int x, float y);
+void foo(float x, int y);
+void foo(float x, float y);
+void foo(int x, int y = 0);
+int main() { foo(^); }
+ )cpp");
+ EXPECT_THAT(
+ Results.signatures,
+ ElementsAre(
+ Sig("foo(int x) -> void", {"int x"}),
+ Sig("foo(int x, int y = 0) -> void", {"int x", "int y = 0"}),
+ Sig("foo(float x, int y) -> void", {"float x", "int y"}),
+ Sig("foo(int x, float y) -> void", {"int x", "float y"}),
+ Sig("foo(float x, float y) -> void", {"float x", "float y"})));
+ // We always prefer the first signature.
+ EXPECT_EQ(0, Results.activeSignature);
+ EXPECT_EQ(0, Results.activeParameter);
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -129,16 +129,16 @@
/// Get the optional chunk as a string. This function is possibly recursive.
///
/// The parameter info for each parameter is appended to the Parameters.
-std::string
-getOptionalParameters(const CodeCompletionString ,
- std::vector ) {
+std::string getOptionalParameters(const CodeCompletionString ,
+ std::vector ,
+ SignatureQualitySignals ) {
std::string Result;
for (const auto : CCS) {
switch (Chunk.Kind) {
case CodeCompletionString::CK_Optional:
assert(Chunk.Optional &&
"Expected the optional code completion string to be non-null.");
- Result += getOptionalParameters(*Chunk.Optional, Parameters);
+ Result += getOptionalParameters(*Chunk.Optional, Parameters, Signal);
break;
case CodeCompletionString::CK_VerticalSpace:
break;
@@ -154,6 +154,8 @@
ParameterInformation Info;
Info.label = Chunk.Text;
Parameters.push_back(std::move(Info));
+ Signal.ContainsActiveParameter = true;
+ Signal.NumberOfOptionalParameters++;
break;
}
default:
@@ -685,6 +687,9 @@
llvm::unique_function ResultsCallback;
};
+using ScoredSignature =
+std::pair;
+
class SignatureHelpCollector final : public CodeCompleteConsumer {
public:
@@ -698,7 +703,9 @@
void ProcessOverloadCandidates(Sema , unsigned CurrentArg,
OverloadCandidate *Candidates,
unsigned NumCandidates) override {
+std::vector ScoredSignatures;
SigHelp.signatures.reserve(NumCandidates);
+ScoredSignatures.reserve(NumCandidates);
// FIXME(rwols): How can we determine the "active overload candidate"?
// Right now the overloaded candidates seem to be provided in a "best fit"
// order, so I'm not too worried about this.
@@ -712,11 +719,45 @@
CurrentArg, S, *Allocator, CCTUInfo, true);
assert(CCS && "Expected the CodeCompletionString to be non-null");
// FIXME: for headers, we need to get a comment from the index.
- SigHelp.signatures.push_back(processOverloadCandidate(
+ ScoredSignatures.push_back(processOverloadCandidate(
Candidate, *CCS,
getParameterDocComment(S.getASTContext(), Candidate, CurrentArg,
/*CommentsFromHeaders=*/false)));
}
+std::sort(ScoredSignatures.begin(), ScoredSignatures.end(),
+ [](const ScoredSignature , const ScoredSignature ) {
+// Ordering follows:
+// - Less number of parameters is better.
+// - Function is better than FunctionType which is better than
+// Function Template.
+// - High score is better.
+// - Shorter signature is better.
+// - Alphebatically smaller is better.
+if (L.first.NumberOfParameters != R.first.NumberOfParameters)
+ return L.first.NumberOfParameters <
+ R.first.NumberOfParameters;
+if (L.first.NumberOfOptionalParameters !=
+R.first.NumberOfOptionalParameters)
+
[PATCH] D50555: [clangd] Introduce scoring mechanism for SignatureInformations.
kadircet updated this revision to Diff 160306.
kadircet marked 2 inline comments as done.
kadircet added a comment.
- Rebase & Resolve discussions.
- Resolve discussions.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50555
Files:
clangd/CodeComplete.cpp
clangd/Quality.cpp
clangd/Quality.h
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1515,6 +1515,28 @@
}
}
+TEST(SignatureHelpTest, OverloadsOrdering) {
+ const auto Results = signatures(R"cpp(
+void foo(int x);
+void foo(int x, float y);
+void foo(float x, int y);
+void foo(float x, float y);
+void foo(int x, int y = 0);
+int main() { foo(^); }
+ )cpp");
+ EXPECT_THAT(
+ Results.signatures,
+ ElementsAre(
+ Sig("foo(int x) -> void", {"int x"}),
+ Sig("foo(int x, int y = 0) -> void", {"int x", "int y = 0"}),
+ Sig("foo(float x, int y) -> void", {"float x", "int y"}),
+ Sig("foo(int x, float y) -> void", {"int x", "float y"}),
+ Sig("foo(float x, float y) -> void", {"float x", "float y"})));
+ // We always prefer the first signature.
+ EXPECT_EQ(0, Results.activeSignature);
+ EXPECT_EQ(0, Results.activeParameter);
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/Quality.h
===
--- clangd/Quality.h
+++ clangd/Quality.h
@@ -163,6 +163,16 @@
/// LSP. (The highest score compares smallest so it sorts at the top).
std::string sortText(float Score, llvm::StringRef Tiebreak = "");
+struct SignatureQualitySignals {
+ uint32_t NumberOfParameters = 0;
+ uint32_t NumberOfOptionalParameters = 0;
+ bool ContainsActiveParameter = false;
+ CodeCompleteConsumer::OverloadCandidate::CandidateKind Kind =
+ CodeCompleteConsumer::OverloadCandidate::CandidateKind::CK_Function;
+};
+llvm::raw_ostream <<(llvm::raw_ostream &,
+ const SignatureQualitySignals &);
+
} // namespace clangd
} // namespace clang
Index: clangd/Quality.cpp
===
--- clangd/Quality.cpp
+++ clangd/Quality.cpp
@@ -401,5 +401,17 @@
return S;
}
+llvm::raw_ostream <<(llvm::raw_ostream ,
+ const SignatureQualitySignals ) {
+ OS << formatv("=== Signature Quality:\n");
+ OS << formatv("\tNumber of parameters: {0}\n", S.NumberOfParameters);
+ OS << formatv("\tNumber of optional parameters: {0}\n",
+S.NumberOfOptionalParameters);
+ OS << formatv("\tContains active parameter: {0}\n",
+S.ContainsActiveParameter);
+ OS << formatv("\tKind: {0}\n", S.Kind);
+ return OS;
+}
+
} // namespace clangd
} // namespace clang
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -129,16 +129,16 @@
/// Get the optional chunk as a string. This function is possibly recursive.
///
/// The parameter info for each parameter is appended to the Parameters.
-std::string
-getOptionalParameters(const CodeCompletionString ,
- std::vector ) {
+std::string getOptionalParameters(const CodeCompletionString ,
+ std::vector ,
+ SignatureQualitySignals ) {
std::string Result;
for (const auto : CCS) {
switch (Chunk.Kind) {
case CodeCompletionString::CK_Optional:
assert(Chunk.Optional &&
"Expected the optional code completion string to be non-null.");
- Result += getOptionalParameters(*Chunk.Optional, Parameters);
+ Result += getOptionalParameters(*Chunk.Optional, Parameters, Signal);
break;
case CodeCompletionString::CK_VerticalSpace:
break;
@@ -154,6 +154,8 @@
ParameterInformation Info;
Info.label = Chunk.Text;
Parameters.push_back(std::move(Info));
+ Signal.ContainsActiveParameter = true;
+ Signal.NumberOfOptionalParameters++;
break;
}
default:
@@ -685,6 +687,9 @@
llvm::unique_function ResultsCallback;
};
+using ScoredSignature =
+std::pair;
+
class SignatureHelpCollector final : public CodeCompleteConsumer {
public:
@@ -698,7 +703,9 @@
void ProcessOverloadCandidates(Sema , unsigned CurrentArg,
OverloadCandidate *Candidates,
unsigned NumCandidates) override {
+std::vector ScoredSignatures;
SigHelp.signatures.reserve(NumCandidates);
+ScoredSignatures.reserve(NumCandidates);
// FIXME(rwols): How can we determine the "active overload candidate"?
// Right now the overloaded candidates seem to be provided in a "best fit"
// order, so I'm not too worried about
[PATCH] D50555: [clangd] Introduce scoring mechanism for SignatureInformations.
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
Thanks! LGTM with a few NITs
Comment at: clangd/CodeComplete.cpp:687
+struct ScoredSignatureGreater {
+ bool operator()(const ScoredSignature , const ScoredSignature ) {
+// Ordering follows:
NIT: Maybe make it a function with a descriptive name, e.g.
`hasBetterSignature`?
We could call it in a lambda, should make the code even clearer.
Comment at: clangd/CodeComplete.cpp:755
+ ScoredSignatureGreater());
+for (const auto : ScoredSignatures) {
+ SigHelp.signatures.push_back(SS.second);
NIT: remove braces around the single-statement loop body
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50555
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50555: [clangd] Introduce scoring mechanism for SignatureInformations.
kadircet updated this revision to Diff 160097.
kadircet marked 3 inline comments as done.
kadircet added a comment.
Herald added a subscriber: mgrang.
- Resolve discussions.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50555
Files:
clangd/CodeComplete.cpp
clangd/Quality.cpp
clangd/Quality.h
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1438,6 +1438,28 @@
}
}
+TEST(SignatureHelpTest, OverloadsOrdering) {
+ const auto Results = signatures(R"cpp(
+void foo(int x);
+void foo(int x, float y);
+void foo(float x, int y);
+void foo(float x, float y);
+void foo(int x, int y = 0);
+int main() { foo(^); }
+ )cpp");
+ EXPECT_THAT(
+ Results.signatures,
+ ElementsAre(
+ Sig("foo(int x) -> void", {"int x"}),
+ Sig("foo(int x, int y = 0) -> void", {"int x", "int y = 0"}),
+ Sig("foo(float x, int y) -> void", {"float x", "int y"}),
+ Sig("foo(int x, float y) -> void", {"int x", "float y"}),
+ Sig("foo(float x, float y) -> void", {"float x", "float y"})));
+ // We always prefer the first signature.
+ EXPECT_EQ(0, Results.activeSignature);
+ EXPECT_EQ(0, Results.activeParameter);
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/Quality.h
===
--- clangd/Quality.h
+++ clangd/Quality.h
@@ -163,6 +163,16 @@
/// LSP. (The highest score compares smallest so it sorts at the top).
std::string sortText(float Score, llvm::StringRef Tiebreak = "");
+struct SignatureQualitySignals {
+ uint32_t NumberOfParameters = 0;
+ uint32_t NumberOfOptionalParameters = 0;
+ bool ContainsActiveParameter = false;
+ CodeCompleteConsumer::OverloadCandidate::CandidateKind Kind =
+ CodeCompleteConsumer::OverloadCandidate::CandidateKind::CK_Function;
+};
+llvm::raw_ostream <<(llvm::raw_ostream &,
+ const SignatureQualitySignals &);
+
} // namespace clangd
} // namespace clang
Index: clangd/Quality.cpp
===
--- clangd/Quality.cpp
+++ clangd/Quality.cpp
@@ -401,5 +401,17 @@
return S;
}
+llvm::raw_ostream <<(llvm::raw_ostream ,
+ const SignatureQualitySignals ) {
+ OS << formatv("=== Signature Quality:\n");
+ OS << formatv("\tNumber of parameters: {0}\n", S.NumberOfParameters);
+ OS << formatv("\tNumber of optional parameters: {0}\n",
+S.NumberOfOptionalParameters);
+ OS << formatv("\tContains active parameter: {0}\n",
+S.ContainsActiveParameter);
+ OS << formatv("\tKind: {0}\n", S.Kind);
+ return OS;
+}
+
} // namespace clangd
} // namespace clang
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -128,16 +128,16 @@
/// Get the optional chunk as a string. This function is possibly recursive.
///
/// The parameter info for each parameter is appended to the Parameters.
-std::string
-getOptionalParameters(const CodeCompletionString ,
- std::vector ) {
+std::string getOptionalParameters(const CodeCompletionString ,
+ std::vector ,
+ SignatureQualitySignals ) {
std::string Result;
for (const auto : CCS) {
switch (Chunk.Kind) {
case CodeCompletionString::CK_Optional:
assert(Chunk.Optional &&
"Expected the optional code completion string to be non-null.");
- Result += getOptionalParameters(*Chunk.Optional, Parameters);
+ Result += getOptionalParameters(*Chunk.Optional, Parameters, Signal);
break;
case CodeCompletionString::CK_VerticalSpace:
break;
@@ -153,6 +153,8 @@
ParameterInformation Info;
Info.label = Chunk.Text;
Parameters.push_back(std::move(Info));
+ Signal.ContainsActiveParameter = true;
+ Signal.NumberOfOptionalParameters++;
break;
}
default:
@@ -679,6 +681,41 @@
llvm::unique_function ResultsCallback;
};
+using ScoredSignature =
+std::pair;
+struct ScoredSignatureGreater {
+ bool operator()(const ScoredSignature , const ScoredSignature ) {
+// Ordering follows:
+// - Less number of parameters is better.
+// - Function is better than FunctionType which is better than Function
+// Template.
+// - High score is better.
+// - Shorter signature is better.
+// - Alphebatically smaller is better.
+if (L.first.NumberOfParameters != R.first.NumberOfParameters)
+ return L.first.NumberOfParameters < R.first.NumberOfParameters;
+if (L.first.NumberOfOptionalParameters !=
+R.first.NumberOfOptionalParameters)
+
[PATCH] D50555: [clangd] Introduce scoring mechanism for SignatureInformations.
ilya-biryukov added inline comments.
Comment at: clangd/CodeComplete.cpp:715
unsigned NumCandidates) override {
+TopN Top(
+std::numeric_limits::max());
Maybe use `vector`, followed by `std::sort` at the end?
Or is there any functionality that we can reuse from the priority queue?
Comment at: clangd/Quality.h:170
+ bool ContainsActiveParameter = false;
+ CodeCompleteConsumer::OverloadCandidate::CandidateKind Kind;
+
Maybe set some default value to avoid UB if someone forgets to set it?
Comment at: clangd/Quality.h:172
+
+ float evaluate() const;
+};
Maybe write a direct comparison here instead of `evaluate`?
Having floating scores makes sense for completion, where lot's of different
signals from multiple sources are taken into account and clients have to
rescore on the client side.
Not so much for SignatureHelp, where we have all required information to
compare the signatures is on clangd side and clients won't need to rescore.
A comparison operator should be easier to reason about, though.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50555
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50555: [clangd] Introduce scoring mechanism for SignatureInformations.
kadircet created this revision.
kadircet added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, ioeric.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50555
Files:
clangd/CodeComplete.cpp
clangd/Quality.cpp
clangd/Quality.h
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1438,6 +1438,28 @@
}
}
+TEST(SignatureHelpTest, OverloadsOrdering) {
+ const auto Results = signatures(R"cpp(
+void foo(int x);
+void foo(int x, float y);
+void foo(float x, int y);
+void foo(float x, float y);
+void foo(int x, int y = 0);
+int main() { foo(^); }
+ )cpp");
+ EXPECT_THAT(
+ Results.signatures,
+ ElementsAre(
+ Sig("foo(int x) -> void", {"int x"}),
+ Sig("foo(int x, int y = 0) -> void", {"int x", "int y = 0"}),
+ Sig("foo(float x, int y) -> void", {"float x", "int y"}),
+ Sig("foo(int x, float y) -> void", {"int x", "float y"}),
+ Sig("foo(float x, float y) -> void", {"float x", "float y"})));
+ // We always prefer the first signature.
+ EXPECT_EQ(0, Results.activeSignature);
+ EXPECT_EQ(0, Results.activeParameter);
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/Quality.h
===
--- clangd/Quality.h
+++ clangd/Quality.h
@@ -163,6 +163,17 @@
/// LSP. (The highest score compares smallest so it sorts at the top).
std::string sortText(float Score, llvm::StringRef Tiebreak = "");
+struct SignatureQualitySignals {
+ uint32_t NumberOfParameters = 0;
+ uint32_t NumberOfOptionalParameters = 0;
+ bool ContainsActiveParameter = false;
+ CodeCompleteConsumer::OverloadCandidate::CandidateKind Kind;
+
+ float evaluate() const;
+};
+llvm::raw_ostream <<(llvm::raw_ostream &,
+ const SignatureQualitySignals &);
+
} // namespace clangd
} // namespace clang
Index: clangd/Quality.cpp
===
--- clangd/Quality.cpp
+++ clangd/Quality.cpp
@@ -401,5 +401,36 @@
return S;
}
+llvm::raw_ostream <<(llvm::raw_ostream ,
+ const SignatureQualitySignals ) {
+ OS << formatv("=== Signature Quality: {0}\n", S.evaluate());
+ OS << formatv("\tNumber of parameters: {0}\n", S.NumberOfParameters);
+ OS << formatv("\tNumber of optional parameters: {0}\n",
+S.NumberOfOptionalParameters);
+ OS << formatv("\tContains active parameter: {0}\n",
+S.ContainsActiveParameter);
+ OS << formatv("\tKind: {0}\n", S.Kind);
+ return OS;
+}
+
+float SignatureQualitySignals::evaluate() const {
+ using OC = CodeCompleteConsumer::OverloadCandidate;
+ float score = 1;
+ score /= NumberOfParameters + 1;
+ if (ContainsActiveParameter)
+score *= 10;
+ switch (Kind) {
+ case OC::CK_Function:
+score *= 10;
+break;
+ case OC::CK_FunctionType:
+score *= 5;
+break;
+ case OC::CK_FunctionTemplate:
+break;
+ }
+ return score;
+}
+
} // namespace clangd
} // namespace clang
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -128,16 +128,16 @@
/// Get the optional chunk as a string. This function is possibly recursive.
///
/// The parameter info for each parameter is appended to the Parameters.
-std::string
-getOptionalParameters(const CodeCompletionString ,
- std::vector ) {
+std::string getOptionalParameters(const CodeCompletionString ,
+ std::vector ,
+ SignatureQualitySignals ) {
std::string Result;
for (const auto : CCS) {
switch (Chunk.Kind) {
case CodeCompletionString::CK_Optional:
assert(Chunk.Optional &&
"Expected the optional code completion string to be non-null.");
- Result += getOptionalParameters(*Chunk.Optional, Parameters);
+ Result += getOptionalParameters(*Chunk.Optional, Parameters, Signal);
break;
case CodeCompletionString::CK_VerticalSpace:
break;
@@ -153,6 +153,8 @@
ParameterInformation Info;
Info.label = Chunk.Text;
Parameters.push_back(std::move(Info));
+ Signal.ContainsActiveParameter = true;
+ Signal.NumberOfOptionalParameters++;
break;
}
default:
@@ -679,6 +681,24 @@
llvm::unique_function ResultsCallback;
};
+using SignatureInformationScore = float;
+using ScoredSignature =
+std::pair;
+struct ScoredSignatureGreater {
+ bool operator()(const ScoredSignature , const ScoredSignature ) {
+// Ordering follows:
+// - High score is better.
+// - Shorter signature is better.
+// -
