[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE340530: [clangd] Suggest code-completions for overriding
base class virtual methods. (authored by kadircet, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D50898?vs=162157=162158#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50898
Files:
clangd/CodeComplete.cpp
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1680,6 +1680,31 @@
}
}
+TEST(CompletionTest, SuggestOverrides) {
+ constexpr const char *const Text(R"cpp(
+ class A {
+ public:
+virtual void vfunc(bool param);
+virtual void vfunc(bool param, int p);
+void func(bool param);
+ };
+ class B : public A {
+ virtual void ttt(bool param) const;
+ void vfunc(bool param, int p) override;
+ };
+ class C : public B {
+ public:
+void vfunc(bool param) override;
+^
+ };
+ )cpp");
+ const auto Results = completions(Text);
+ EXPECT_THAT(Results.Completions,
+ AllOf(Contains(Labeled("void vfunc(bool param, int p) override")),
+Contains(Labeled("void ttt(bool param) const override")),
+Not(Contains(Labeled("void vfunc(bool param) override");
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -188,14 +188,66 @@
return HeaderFile{std::move(*Resolved), /*Verbatim=*/false};
}
+// First traverses all method definitions inside current class/struct/union
+// definition. Than traverses base classes to find virtual methods that haven't
+// been overriden within current context.
+// FIXME(kadircet): Currently we cannot see declarations below completion point.
+// It is because Sema gets run only upto completion point. Need to find a
+// solution to run it for the whole class/struct/union definition.
+static std::vector
+getNonOverridenMethodCompletionResults(const DeclContext *DC, Sema *S) {
+ const auto *CR = llvm::dyn_cast(DC);
+ // If not inside a class/struct/union return empty.
+ if (!CR)
+return {};
+ // First store overrides within current class.
+ // These are stored by name to make querying fast in the later step.
+ llvm::StringMap> Overrides;
+ for (auto *Method : CR->methods()) {
+if (!Method->isVirtual())
+ continue;
+Overrides[Method->getName()].push_back(Method);
+ }
+
+ std::vector Results;
+ for (const auto : CR->bases()) {
+const auto *BR = Base.getType().getTypePtr()->getAsCXXRecordDecl();
+if (!BR)
+ continue;
+for (auto *Method : BR->methods()) {
+ if (!Method->isVirtual())
+continue;
+ const auto it = Overrides.find(Method->getName());
+ bool IsOverriden = false;
+ if (it != Overrides.end()) {
+for (auto *MD : it->second) {
+ // If the method in current body is not an overload of this virtual
+ // function, that it overrides this one.
+ if (!S->IsOverload(MD, Method, false)) {
+IsOverriden = true;
+break;
+ }
+}
+ }
+ if (!IsOverriden)
+Results.emplace_back(Method, 0);
+}
+ }
+
+ return Results;
+}
+
/// A code completion result, in clang-native form.
/// It may be promoted to a CompletionItem if it's among the top-ranked results.
struct CompletionCandidate {
llvm::StringRef Name; // Used for filtering and sorting.
// We may have a result from Sema, from the index, or both.
const CodeCompletionResult *SemaResult = nullptr;
const Symbol *IndexResult = nullptr;
+ // States whether this item is an override suggestion.
+ bool IsOverride = false;
+
// Returns a token identifying the overload set this is part of.
// 0 indicates it's not part of any overload set.
size_t overloadSet() const {
@@ -352,21 +404,30 @@
Completion.Documentation = getDocComment(ASTCtx, *C.SemaResult,
/*CommentsFromHeader=*/false);
}
+if (C.IsOverride)
+ S.OverrideSuffix = true;
}
CodeCompletion build() {
Completion.ReturnType = summarizeReturnType();
Completion.Signature = summarizeSignature();
Completion.SnippetSuffix = summarizeSnippet();
Completion.BundleSize = Bundled.size();
+if (summarizeOverride()) {
+ Completion.Name = Completion.ReturnType + ' ' +
+std::move(Completion.Name) +
+std::move(Completion.Signature) + " override";
+ Completion.Signature.clear();
+}
return std::move(Completion);
}
private:
struct BundledEntry {
std::string SnippetSuffix;
std::string Signature;
[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.
hokein accepted this revision. hokein added a comment. still LG. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.
kadircet updated this revision to Diff 162157.
kadircet added a comment.
- Resolve discussions.
- Move override generation logic from render to item generation so that
internal clients can use it as well, also use a boolean for checking override
status of a bundle to be more efficient.
- Change unittests accordingly, get rid of unused IsOverride field.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50898
Files:
clangd/CodeComplete.cpp
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1680,6 +1680,31 @@
}
}
+TEST(CompletionTest, SuggestOverrides) {
+ constexpr const char *const Text(R"cpp(
+ class A {
+ public:
+virtual void vfunc(bool param);
+virtual void vfunc(bool param, int p);
+void func(bool param);
+ };
+ class B : public A {
+ virtual void ttt(bool param) const;
+ void vfunc(bool param, int p) override;
+ };
+ class C : public B {
+ public:
+void vfunc(bool param) override;
+^
+ };
+ )cpp");
+ const auto Results = completions(Text);
+ EXPECT_THAT(Results.Completions,
+ AllOf(Contains(Labeled("void vfunc(bool param, int p) override")),
+Contains(Labeled("void ttt(bool param) const override")),
+Not(Contains(Labeled("void vfunc(bool param) override");
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -188,14 +188,66 @@
return HeaderFile{std::move(*Resolved), /*Verbatim=*/false};
}
+// First traverses all method definitions inside current class/struct/union
+// definition. Than traverses base classes to find virtual methods that haven't
+// been overriden within current context.
+// FIXME(kadircet): Currently we cannot see declarations below completion point.
+// It is because Sema gets run only upto completion point. Need to find a
+// solution to run it for the whole class/struct/union definition.
+static std::vector
+getNonOverridenMethodCompletionResults(const DeclContext *DC, Sema *S) {
+ const auto *CR = llvm::dyn_cast(DC);
+ // If not inside a class/struct/union return empty.
+ if (!CR)
+return {};
+ // First store overrides within current class.
+ // These are stored by name to make querying fast in the later step.
+ llvm::StringMap> Overrides;
+ for (auto *Method : CR->methods()) {
+if (!Method->isVirtual())
+ continue;
+Overrides[Method->getName()].push_back(Method);
+ }
+
+ std::vector Results;
+ for (const auto : CR->bases()) {
+const auto *BR = Base.getType().getTypePtr()->getAsCXXRecordDecl();
+if (!BR)
+ continue;
+for (auto *Method : BR->methods()) {
+ if (!Method->isVirtual())
+continue;
+ const auto it = Overrides.find(Method->getName());
+ bool IsOverriden = false;
+ if (it != Overrides.end()) {
+for (auto *MD : it->second) {
+ // If the method in current body is not an overload of this virtual
+ // function, that it overrides this one.
+ if (!S->IsOverload(MD, Method, false)) {
+IsOverriden = true;
+break;
+ }
+}
+ }
+ if (!IsOverriden)
+Results.emplace_back(Method, 0);
+}
+ }
+
+ return Results;
+}
+
/// A code completion result, in clang-native form.
/// It may be promoted to a CompletionItem if it's among the top-ranked results.
struct CompletionCandidate {
llvm::StringRef Name; // Used for filtering and sorting.
// We may have a result from Sema, from the index, or both.
const CodeCompletionResult *SemaResult = nullptr;
const Symbol *IndexResult = nullptr;
+ // States whether this item is an override suggestion.
+ bool IsOverride = false;
+
// Returns a token identifying the overload set this is part of.
// 0 indicates it's not part of any overload set.
size_t overloadSet() const {
@@ -352,21 +404,30 @@
Completion.Documentation = getDocComment(ASTCtx, *C.SemaResult,
/*CommentsFromHeader=*/false);
}
+if (C.IsOverride)
+ S.OverrideSuffix = true;
}
CodeCompletion build() {
Completion.ReturnType = summarizeReturnType();
Completion.Signature = summarizeSignature();
Completion.SnippetSuffix = summarizeSnippet();
Completion.BundleSize = Bundled.size();
+if (summarizeOverride()) {
+ Completion.Name = Completion.ReturnType + ' ' +
+std::move(Completion.Name) +
+std::move(Completion.Signature) + " override";
+ Completion.Signature.clear();
+}
return std::move(Completion);
}
private:
struct BundledEntry {
std::string
[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.
kadircet updated this revision to Diff 162151.
kadircet added a comment.
- Resolve discussions.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50898
Files:
clangd/CodeComplete.cpp
clangd/CodeComplete.h
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -61,6 +61,7 @@
MATCHER(InsertInclude, "") { return bool(arg.HeaderInsertion); }
MATCHER_P(SnippetSuffix, Text, "") { return arg.SnippetSuffix == Text; }
MATCHER_P(Origin, OriginSet, "") { return arg.Origin == OriginSet; }
+MATCHER(IsOverride, "") { return bool(arg.IsOverride); }
// Shorthand for Contains(Named(Name)).
Matcher &> Has(std::string Name) {
@@ -1700,6 +1701,58 @@
}
}
+TEST(CompletionTest, SuggestOverrides) {
+ constexpr const char *const Text(R"cpp(
+ class A {
+ public:
+virtual void vfunc(bool param);
+virtual void vfunc(bool param, int p);
+void func(bool param);
+ };
+ class B : public A {
+ virtual void ttt(bool param);
+ void vfunc(bool param, int p) override;
+ };
+ class C : public B {
+ public:
+void vfunc(bool param) override;
+^
+ };
+ )cpp");
+ const auto Results = completions(Text);
+ EXPECT_THAT(Results.Completions,
+ AllOf(Contains(AllOf(Named("vfunc"), IsOverride(),
+ Labeled("vfunc(bool param, int p)"))),
+Contains(AllOf(Named("ttt"), IsOverride(),
+ Labeled("ttt(bool param)"))),
+Not(Contains(AllOf(Named("vfunc"), IsOverride(),
+ Labeled("vfunc(bool param)"));
+}
+
+TEST(CompletionTest, RenderOverride) {
+ CodeCompletion C;
+ C.Name = "x";
+ C.Signature = "(bool) const";
+ C.SnippetSuffix = "(${0:bool})";
+ C.ReturnType = "int";
+ C.Documentation = "This is x().";
+ C.Kind = CompletionItemKind::Method;
+ C.Score.Total = 1.0;
+ C.Origin = SymbolOrigin::AST;
+ C.IsOverride = true;
+
+ CodeCompleteOptions Opts;
+ Opts.IncludeIndicator.NoInsert = "";
+ auto R = C.render(Opts);
+ EXPECT_EQ(R.label, "int x(bool) const override");
+ EXPECT_EQ(R.insertText, "int x(bool) const override");
+ EXPECT_EQ(R.insertTextFormat, InsertTextFormat::PlainText);
+ EXPECT_EQ(R.filterText, "x");
+ EXPECT_EQ(R.detail, "int");
+ EXPECT_EQ(R.documentation, "This is x().");
+ EXPECT_THAT(R.additionalTextEdits, IsEmpty());
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/CodeComplete.h
===
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -131,6 +131,9 @@
/// Holds the range of the token we are going to replace with this completion.
Range CompletionTokenRange;
+ /// Whether this completion is for overriding a virtual method.
+ bool IsOverride = false;
+
// Scores are used to rank completion items.
struct Scores {
// The score that items are ranked by.
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -188,14 +188,66 @@
return HeaderFile{std::move(*Resolved), /*Verbatim=*/false};
}
+// First traverses all method definitions inside current class/struct/union
+// definition. Than traverses base classes to find virtual methods that haven't
+// been overriden within current context.
+// FIXME(kadircet): Currently we cannot see declarations below completion point.
+// It is because Sema gets run only upto completion point. Need to find a
+// solution to run it for the whole class/struct/union definition.
+static std::vector
+getNonOverridenMethodCompletionResults(const DeclContext *DC, Sema *S) {
+ const auto *CR = llvm::dyn_cast(DC);
+ // If not inside a class/struct/union return empty.
+ if (!CR)
+return {};
+ // First store overrides within current class.
+ // These are stored by name to make querying fast in the later step.
+ llvm::StringMap> Overrides;
+ for (auto *Method : CR->methods()) {
+if (!Method->isVirtual())
+ continue;
+Overrides[Method->getName()].push_back(Method);
+ }
+
+ std::vector Results;
+ for (const auto : CR->bases()) {
+const auto *BR = Base.getType().getTypePtr()->getAsCXXRecordDecl();
+if (!BR)
+ continue;
+for (auto *Method : BR->methods()) {
+ if (!Method->isVirtual())
+continue;
+ const auto it = Overrides.find(Method->getName());
+ bool IsOverriden = false;
+ if (it != Overrides.end()) {
+for (auto *MD : it->second) {
+ // If the method in current body is not an overload of this virtual
+ // function, that it overrides this one.
+ if (!S->IsOverload(MD, Method, false)) {
+IsOverriden = true;
+break;
+ }
+}
+
[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.
hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land. Thanks, LGTM. Comment at: clangd/CodeComplete.cpp:210 +const std::string Name = Method->getNameAsString(); +Overrides[Name].push_back(Method); + } nit: here as well, use `Overrides[Method->getName()].push_back...` Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.
kadircet updated this revision to Diff 161963.
kadircet marked 6 inline comments as done.
kadircet added a comment.
- Resolve discussions.
- Fix a bug inside summarizeOverride.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50898
Files:
clangd/CodeComplete.cpp
clangd/CodeComplete.h
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -61,6 +61,7 @@
MATCHER(InsertInclude, "") { return bool(arg.HeaderInsertion); }
MATCHER_P(SnippetSuffix, Text, "") { return arg.SnippetSuffix == Text; }
MATCHER_P(Origin, OriginSet, "") { return arg.Origin == OriginSet; }
+MATCHER(IsOverride, "") { return bool(arg.IsOverride); }
// Shorthand for Contains(Named(Name)).
Matcher &> Has(std::string Name) {
@@ -1700,6 +1701,58 @@
}
}
+TEST(CompletionTest, SuggestOverrides) {
+ constexpr const char *const Text(R"cpp(
+ class A {
+ public:
+virtual void vfunc(bool param);
+virtual void vfunc(bool param, int p);
+void func(bool param);
+ };
+ class B : public A {
+ virtual void ttt(bool param);
+ void vfunc(bool param, int p) override;
+ };
+ class C : public B {
+ public:
+void vfunc(bool param) override;
+^
+ };
+ )cpp");
+ const auto Results = completions(Text);
+ EXPECT_THAT(Results.Completions,
+ AllOf(Contains(AllOf(Named("vfunc"), IsOverride(),
+ Labeled("vfunc(bool param, int p)"))),
+Contains(AllOf(Named("ttt"), IsOverride(),
+ Labeled("ttt(bool param)"))),
+Not(Contains(AllOf(Named("vfunc"), IsOverride(),
+ Labeled("vfunc(bool param)"));
+}
+
+TEST(CompletionTest, RenderOverride) {
+ CodeCompletion C;
+ C.Name = "x";
+ C.Signature = "(bool) const";
+ C.SnippetSuffix = "(${0:bool})";
+ C.ReturnType = "int";
+ C.Documentation = "This is x().";
+ C.Kind = CompletionItemKind::Method;
+ C.Score.Total = 1.0;
+ C.Origin = SymbolOrigin::AST;
+ C.IsOverride = true;
+
+ CodeCompleteOptions Opts;
+ Opts.IncludeIndicator.NoInsert = "";
+ auto R = C.render(Opts);
+ EXPECT_EQ(R.label, "int x(bool) const override");
+ EXPECT_EQ(R.insertText, "int x(bool) const override");
+ EXPECT_EQ(R.insertTextFormat, InsertTextFormat::PlainText);
+ EXPECT_EQ(R.filterText, "x");
+ EXPECT_EQ(R.detail, "int");
+ EXPECT_EQ(R.documentation, "This is x().");
+ EXPECT_THAT(R.additionalTextEdits, IsEmpty());
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/CodeComplete.h
===
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -131,6 +131,9 @@
/// Holds the range of the token we are going to replace with this completion.
Range CompletionTokenRange;
+ /// Whether this completion is for overriding a virtual method.
+ bool IsOverride = false;
+
// Scores are used to rank completion items.
struct Scores {
// The score that items are ranked by.
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -188,14 +188,67 @@
return HeaderFile{std::move(*Resolved), /*Verbatim=*/false};
}
+// First traverses all method definitions inside current class/struct/union
+// definition. Than traverses base classes to find virtual methods that haven't
+// been overriden within current context.
+// FIXME(kadircet): Currently we cannot see declarations below completion point.
+// It is because Sema gets run only upto completion point. Need to find a
+// solution to run it for the whole class/struct/union definition.
+static std::vector
+getNonOverridenMethodCompletionResults(const DeclContext *DC, Sema *S) {
+ const auto *CR = llvm::dyn_cast(DC);
+ // If not inside a class/struct/union return empty.
+ if (!CR)
+return {};
+ // First store overrides within current class.
+ // These are stored by name to make querying fast in the later step.
+ llvm::StringMap> Overrides;
+ for (auto *Method : CR->methods()) {
+if (!Method->isVirtual())
+ continue;
+const std::string Name = Method->getNameAsString();
+Overrides[Name].push_back(Method);
+ }
+
+ std::vector Results;
+ for (const auto : CR->bases()) {
+const auto *BR = Base.getType().getTypePtr()->getAsCXXRecordDecl();
+if (!BR)
+ continue;
+for (auto *Method : BR->methods()) {
+ if (!Method->isVirtual())
+continue;
+ const auto it = Overrides.find(Method->getName());
+ bool IsOverriden = false;
+ if (it != Overrides.end()) {
+for (auto *MD : it->second) {
+ // If the method in current body is not an overload of this virtual
+ // function, that it overrides this one.
+
[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.
hokein added a comment.
Looks good mostly, a few nits. We should make sure all related comments are
updated accordingly
Comment at: clangd/CodeComplete.cpp:198
+static std::vector
+getVirtualNonOverridenMethods(const DeclContext *DC, Sema *S) {
+ const auto *CR = llvm::dyn_cast(DC);
Since we are returning `CodeCompletionResult`, maybe naming it like
`getNonOverridenCompleteionResults` is clearer?
Comment at: clangd/CodeComplete.cpp:222
+ const std::string Name = Method->getNameAsString();
+ const auto it = Overrides.find(Name);
+ bool IsOverriden = false;
nit: Can't we use `Overrides.find(Method->getName())` and the other places as
well?
Comment at: clangd/CodeComplete.cpp:224
+ bool IsOverriden = false;
+ if (it != Overrides.end())
+for (auto *MD : it->second) {
nit: use `{}` around the body of `if` -- the one-line for statement is across
line, adding `{}` around it will improve the readability.
Comment at: clangd/CodeComplete.cpp:1238
: SymbolSlab();
// Merge Sema and Index results, score them, and pick the winners.
+const auto Overrides = getVirtualNonOverridenMethods(
nit: we need to update the comment accordingly.
Comment at: clangd/CodeComplete.cpp:1281
// Groups overloads if desired, to form CompletionCandidate::Bundles.
// The bundles are scored and top results are returned, best to worst.
std::vector
here as well.
Comment at: clangd/CodeComplete.cpp:1322
AddToBundles(, CorrespondingIndexResult(SemaResult));
+for (auto : OverrideResults)
+ AddToBundles(, CorrespondingIndexResult(OverrideResult),
IIUC, we are treating the override results the same `SemaResult`, it is safe as
long as Sema doesn't provide these overrides in its code completion results,
otherwise we will have duplicated completion items?
I think we probably need a proper comment explaining what's going on here.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50898
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.
kadircet added a comment. After today's offline discussion I suppose we are not going to implement it within Sema. And also I think getVirtualNonOverridenMethods is a pretty generic function that has no ties to clangd, so it can be easily moved into Sema. Should we still move it into a separate file or leave it there? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.
ioeric added a comment. In https://reviews.llvm.org/D50898#1205756, @hokein wrote: > I think it is reasonable to make Sema support suggesting override methods, > instead of implementing it in clangd side? drive-by: +1 to this. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.
kadircet marked an inline comment as done. kadircet added inline comments. Comment at: clangd/CodeComplete.cpp:1233 +// struct/class/union definition. +const auto Overrides = getVirtualNonOverridenMethods( +Recorder->CCSema->CurContext, Recorder->CCSema); hokein wrote: > It seems that we treat it as a special case, the code path here runs out of > the `ranking` path. Put override suggestions to ranking system. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.
kadircet updated this revision to Diff 161683.
kadircet marked 3 inline comments as done.
kadircet added a comment.
- Put overrides through scoring and resolve nits.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50898
Files:
clangd/CodeComplete.cpp
clangd/CodeComplete.h
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -61,6 +61,7 @@
MATCHER(InsertInclude, "") { return bool(arg.HeaderInsertion); }
MATCHER_P(SnippetSuffix, Text, "") { return arg.SnippetSuffix == Text; }
MATCHER_P(Origin, OriginSet, "") { return arg.Origin == OriginSet; }
+MATCHER(IsOverride, "") { return bool(arg.IsOverride); }
// Shorthand for Contains(Named(Name)).
Matcher &> Has(std::string Name) {
@@ -1648,6 +1649,58 @@
SigDoc("Doc from sema";
}
+TEST(CompletionTest, SuggestOverrides) {
+ constexpr const char *const Text(R"cpp(
+ class A {
+ public:
+virtual void vfunc(bool param);
+virtual void vfunc(bool param, int p);
+void func(bool param);
+ };
+ class B : public A {
+ virtual void ttt(bool param);
+ void vfunc(bool param, int p) override;
+ };
+ class C : public B {
+ public:
+void vfunc(bool param) override;
+^
+ };
+ )cpp");
+ const auto Results = completions(Text);
+ EXPECT_THAT(Results.Completions,
+ AllOf(Contains(AllOf(Named("vfunc"), IsOverride(),
+ Labeled("vfunc(bool param, int p)"))),
+Contains(AllOf(Named("ttt"), IsOverride(),
+ Labeled("ttt(bool param)"))),
+Not(Contains(AllOf(Named("vfunc"), IsOverride(),
+ Labeled("vfunc(bool param)"));
+}
+
+TEST(CompletionTest, RenderOverride) {
+ CodeCompletion C;
+ C.Name = "x";
+ C.Signature = "(bool) const";
+ C.SnippetSuffix = "(${0:bool})";
+ C.ReturnType = "int";
+ C.Documentation = "This is x().";
+ C.Kind = CompletionItemKind::Method;
+ C.Score.Total = 1.0;
+ C.Origin = SymbolOrigin::AST;
+ C.IsOverride = true;
+
+ CodeCompleteOptions Opts;
+ Opts.IncludeIndicator.NoInsert = "";
+ auto R = C.render(Opts);
+ EXPECT_EQ(R.label, "int x(bool) const override");
+ EXPECT_EQ(R.insertText, "int x(bool) const override");
+ EXPECT_EQ(R.insertTextFormat, InsertTextFormat::PlainText);
+ EXPECT_EQ(R.filterText, "x");
+ EXPECT_EQ(R.detail, "int");
+ EXPECT_EQ(R.documentation, "This is x().");
+ EXPECT_THAT(R.additionalTextEdits, IsEmpty());
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/CodeComplete.h
===
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -127,6 +127,9 @@
/// Holds the range of the token we are going to replace with this completion.
Range CompletionTokenRange;
+ /// Whether this completion is for overriding a virtual method.
+ bool IsOverride = false;
+
// Scores are used to rank completion items.
struct Scores {
// The score that items are ranked by.
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -188,14 +188,67 @@
return HeaderFile{std::move(*Resolved), /*Verbatim=*/false};
}
+// First traverses all method definitions inside current class/struct/union
+// definition. Than traverses base classes to find virtual methods that haven't
+// been overriden within current context.
+// FIXME(kadircet): Currently we cannot see declarations below completion point.
+// It is because Sema gets run only upto completion point. Need to find a
+// solution to run it for the whole class/struct/union definition.
+static std::vector
+getVirtualNonOverridenMethods(const DeclContext *DC, Sema *S) {
+ const auto *CR = llvm::dyn_cast(DC);
+ // If not inside a class/struct/union return empty.
+ if (!CR)
+return {};
+ // First store overrides within current class.
+ // These are stored by name to make querying fast in the later step.
+ llvm::StringMap> Overrides;
+ for (auto *Method : CR->methods()) {
+if (!Method->isVirtual())
+ continue;
+const std::string Name = Method->getNameAsString();
+Overrides[Name].push_back(Method);
+ }
+
+ std::vector Results;
+ for (const auto : CR->bases()) {
+const auto *BR = Base.getType().getTypePtr()->getAsCXXRecordDecl();
+if (!BR)
+ continue;
+for (auto *Method : BR->methods()) {
+ if (!Method->isVirtual())
+continue;
+ const std::string Name = Method->getNameAsString();
+ const auto it = Overrides.find(Name);
+ bool IsOverriden = false;
+ if (it != Overrides.end())
+for (auto *MD : it->second) {
+ // If the method in current body is not an overload of
[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.
hokein added a comment.
I think it is reasonable to make Sema support suggesting override methods,
instead of implementing it in clangd side?
Comment at: clangd/CodeComplete.cpp:206
+ llvm::StringMap> Overrides;
+ for (auto *Method : dyn_cast(DC)->methods()) {
+if (!Method->isVirtual())
nit: CR->methods().
Comment at: clangd/CodeComplete.cpp:210
+const std::string Name = Method->getNameAsString();
+const auto it = Overrides.find(Name);
+if (it == Overrides.end())
nit: we can simplify the code like `Overrides[Name].push_back(Method)`.
Comment at: clangd/CodeComplete.cpp:219
+ for (const auto : CR->bases()) {
+const auto *BR = Base.getType().getTypePtr()->getAsCXXRecordDecl();
+for (auto *Method : BR->methods()) {
I think we should check whether `BR == nullptr` here.
Comment at: clangd/CodeComplete.cpp:1233
+// struct/class/union definition.
+const auto Overrides = getVirtualNonOverridenMethods(
+Recorder->CCSema->CurContext, Recorder->CCSema);
It seems that we treat it as a special case, the code path here runs out of the
`ranking` path.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50898
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
