[clangd] Add tweak to override pure virtuals #139348
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clangd Author: Marco Maia (marcogmaia) ChangesThis PR solves the following:
Patch is 24.57 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/139348.diff 4 Files Affected:
diff --git a/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt b/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
index 59475b0dfd3d2..1d6e38088ad67 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
+++ b/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
@@ -14,9 +14,9 @@ set(LLVM_LINK_COMPONENTS
add_clang_library(clangDaemonTweaks OBJECT
AddUsing.cpp
AnnotateHighlightings.cpp
- DumpAST.cpp
DefineInline.cpp
DefineOutline.cpp
+ DumpAST.cpp
ExpandDeducedType.cpp
ExpandMacro.cpp
ExtractFunction.cpp
@@ -24,6 +24,7 @@ add_clang_library(clangDaemonTweaks OBJECT
MemberwiseConstructor.cpp
ObjCLocalizeStringLiteral.cpp
ObjCMemberwiseInitializer.cpp
+ OverridePureVirtuals.cpp
PopulateSwitch.cpp
RawStringLiteral.cpp
RemoveUsingNamespace.cpp
diff --git a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
new file mode 100644
index 0000000000000..b8880433fdd52
--- /dev/null
+++ b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
@@ -0,0 +1,366 @@
+//===--- AddPureVirtualOverride.cpp ------------------------------*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "refactor/Tweak.h"
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/Type.h"
+#include "clang/AST/TypeLoc.h"
+#include "clang/Basic/LLVM.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/DenseSet.h"
+#include "llvm/Support/FormatVariadic.h"
+#include <algorithm>
+#include <functional>
+#include <map>
+#include <set>
+#include <string>
+#include <vector>
+
+namespace clang {
+namespace clangd {
+namespace {
+
+class OverridePureVirtuals : public Tweak {
+public:
+ const char *id() const final; // defined by REGISTER_TWEAK.
+ bool prepare(const Selection &Sel) override;
+ Expected<Effect> apply(const Selection &Sel) override;
+ std::string title() const override { return "Override pure virtual methods"; }
+ llvm::StringLiteral kind() const override {
+ return CodeAction::REFACTOR_KIND;
+ }
+
+private:
+ // Stores the CXXRecordDecl of the class being modified.
+ const CXXRecordDecl *CurrentDecl = nullptr;
+ // Stores pure virtual methods that need overriding, grouped by their original
+ // access specifier.
+ std::map<AccessSpecifier, std::vector<const CXXMethodDecl *>>
+ MissingMethodsByAccess;
+ // Stores the source locations of existing access specifiers in CurrentDecl.
+ std::map<AccessSpecifier, SourceLocation> AccessSpecifierLocations;
+
+ // Helper function to gather information before applying the tweak.
+ void collectMissingPureVirtuals(const Selection &Sel);
+};
+
+REGISTER_TWEAK(OverridePureVirtuals)
+
+// Collects all unique, canonical pure virtual methods from a class and its
+// entire inheritance hierarchy. This function aims to find methods that *could*
+// make a derived class abstract if not implemented.
+std::vector<const CXXMethodDecl *>
+getAllUniquePureVirtualsFromHierarchy(const CXXRecordDecl *Decl) {
+ std::vector<const CXXMethodDecl *> Result;
+ llvm::DenseSet<const CXXMethodDecl *> VisitedCanonicalMethods;
+ // We declare it as a std::function because we are going to call it
+ // recursively.
+ std::function<void(const CXXRecordDecl *)> Collect;
+
+ Collect = [&](const CXXRecordDecl *CurrentClass) {
+ if (!CurrentClass) {
+ return;
+ }
+ const CXXRecordDecl *Def = CurrentClass->getDefinition();
+ if (!Def) {
+ return;
+ }
+
+ for (const CXXMethodDecl *M : Def->methods()) {
+ // Add if its canonical declaration hasn't been processed yet.
+ // This ensures each distinct pure virtual method signature is collected
+ // once.
+ if (M->isPureVirtual() &&
+ VisitedCanonicalMethods.insert(M->getCanonicalDecl()).second) {
+ Result.emplace_back(M); // Store the specific declaration encountered.
+ }
+ }
+
+ for (const auto &BaseSpec : Def->bases()) {
+ if (const CXXRecordDecl *BaseDef =
+ BaseSpec.getType()->getAsCXXRecordDecl()) {
+ Collect(BaseDef); // Recursively collect from base classes.
+ }
+ }
+ };
+
+ Collect(Decl);
+ return Result;
+}
+
+// Gets canonical declarations of methods already overridden or implemented in
+// class D.
+std::set<const CXXMethodDecl *>
+getImplementedOrOverriddenCanonicals(const CXXRecordDecl *D) {
+ std::set<const CXXMethodDecl *> ImplementedSet;
+ for (const CXXMethodDecl *M : D->methods()) {
+ // If M provides an implementation for any virtual method it overrides.
+ // A method is an "implementation" if it's virtual and not pure.
+ // Or if it directly overrides a base method.
+ for (const CXXMethodDecl *OverriddenM : M->overridden_methods()) {
+ ImplementedSet.insert(OverriddenM->getCanonicalDecl());
+ }
+ }
+ return ImplementedSet;
+}
+
+// Get the location of every colon of the `AccessSpecifier`.
+std::map<AccessSpecifier, SourceLocation>
+getSpecifierLocations(const CXXRecordDecl *D) {
+ std::map<AccessSpecifier, SourceLocation> Locs;
+ for (auto *DeclNode : D->decls()) { // Changed to DeclNode to avoid ambiguity
+ if (const auto *ASD = llvm::dyn_cast<AccessSpecDecl>(DeclNode)) {
+ Locs[ASD->getAccess()] = ASD->getColonLoc();
+ }
+ }
+ return Locs;
+}
+
+// Check if the current class has any pure virtual method to be implemented.
+bool OverridePureVirtuals::prepare(const Selection &Sel) {
+ const SelectionTree::Node *Node = Sel.ASTSelection.commonAncestor();
+ if (!Node) {
+ return false;
+ }
+
+ // Make sure we have a definition.
+ CurrentDecl = Node->ASTNode.get<CXXRecordDecl>();
+ if (!CurrentDecl || !CurrentDecl->getDefinition()) {
+ return false;
+ }
+
+ // A class needs overrides if it's abstract itself, or derives from abstract
+ // bases and hasn't implemented all necessary methods. A simpler check: if it
+ // has any base that is abstract.
+ bool HasAbstractBase = false;
+ for (const auto &Base : CurrentDecl->bases()) {
+ if (const CXXRecordDecl *BaseDecl = Base.getType()->getAsCXXRecordDecl()) {
+ if (BaseDecl->getDefinition() &&
+ BaseDecl->getDefinition()->isAbstract()) {
+ HasAbstractBase = true;
+ break;
+ }
+ }
+ }
+
+ // Only offer for polymorphic classes with abstract bases.
+ return CurrentDecl->isPolymorphic() && HasAbstractBase;
+}
+
+// Collects all pure virtual methods that are missing an override in
+// CurrentDecl, grouped by their original access specifier.
+void OverridePureVirtuals::collectMissingPureVirtuals(const Selection &Sel) {
+ if (!CurrentDecl)
+ return;
+ CurrentDecl = CurrentDecl->getDefinition(); // Work with the definition
+ if (!CurrentDecl)
+ return;
+
+ AccessSpecifierLocations = getSpecifierLocations(CurrentDecl);
+ MissingMethodsByAccess.clear();
+
+ // Get all unique pure virtual methods from the entire base class hierarchy.
+ std::vector<const CXXMethodDecl *> AllPureVirtualsInHierarchy;
+ llvm::DenseSet<const CXXMethodDecl *> CanonicalPureVirtualsSeen;
+
+ for (const auto &BaseSpec : CurrentDecl->bases()) {
+ if (const CXXRecordDecl *BaseRD =
+ BaseSpec.getType()->getAsCXXRecordDecl()) {
+ const CXXRecordDecl *BaseDef = BaseRD->getDefinition();
+ if (!BaseDef)
+ continue;
+
+ std::vector<const CXXMethodDecl *> PuresFromBasePath =
+ getAllUniquePureVirtualsFromHierarchy(BaseDef);
+ for (const CXXMethodDecl *M : PuresFromBasePath) {
+ if (CanonicalPureVirtualsSeen.insert(M->getCanonicalDecl()).second) {
+ AllPureVirtualsInHierarchy.emplace_back(M);
+ }
+ }
+ }
+ }
+
+ // Get methods already implemented or overridden in CurrentDecl.
+ const auto ImplementedOrOverriddenSet =
+ getImplementedOrOverriddenCanonicals(CurrentDecl);
+
+ // Filter AllPureVirtualsInHierarchy to find those not in
+ // ImplementedOrOverriddenSet.
+ for (const CXXMethodDecl *BaseMethod : AllPureVirtualsInHierarchy) {
+ bool AlreadyHandled =
+ ImplementedOrOverriddenSet.count(BaseMethod->getCanonicalDecl()) > 0;
+
+ if (!AlreadyHandled) {
+ // This method needs an override.
+ // Group it by its access specifier in its defining class.
+ MissingMethodsByAccess[BaseMethod->getAccess()].emplace_back(BaseMethod);
+ }
+ }
+}
+
+// Free function to generate the string for a group of method overrides.
+std::string
+generateOverridesStringForGroup(std::vector<const CXXMethodDecl *> Methods,
+ const LangOptions &LangOpts) {
+ const auto GetParamString = [&LangOpts](const ParmVarDecl *P) {
+ std::string TypeStr = P->getType().getAsString(LangOpts);
+ if (P->getNameAsString().empty()) {
+ // Unnamed parameter.
+ return TypeStr;
+ }
+ return llvm::formatv("{0} {1}", TypeStr, P->getNameAsString()).str();
+ };
+
+ std::string MethodsString;
+ for (const auto *Method : Methods) {
+ std::vector<std::string> ParamsAsString;
+ ParamsAsString.reserve(Method->parameters().size());
+ std::transform(Method->param_begin(), Method->param_end(),
+ std::back_inserter(ParamsAsString), GetParamString);
+ const auto Params = llvm::join(ParamsAsString, ", ");
+
+ MethodsString +=
+ llvm::formatv(
+ " {0} {1}({2}) {3}override {{\n"
+ " // TODO: Implement this pure virtual method\n"
+ " static_assert(false, \"Method `{1}` is not implemented.\");\n"
+ " }\n",
+ Method->getReturnType().getAsString(LangOpts),
+ Method->getNameAsString(), Params,
+ std::string(Method->isConst() ? "const " : ""))
+ .str();
+ }
+ return MethodsString;
+}
+
+// Helper to get the string spelling of an AccessSpecifier.
+std::string getAccessSpecifierSpelling(AccessSpecifier AS) {
+ switch (AS) {
+ case AS_public:
+ return "public";
+ case AS_protected:
+ return "protected";
+ case AS_private:
+ return "private";
+ case AS_none:
+ // Should not typically occur for class members.
+ return "";
+ }
+ // Unreachable.
+ return "";
+}
+
+Expected<Tweak::Effect> OverridePureVirtuals::apply(const Selection &Sel) {
+ // The correctness of this tweak heavily relies on the accurate population of
+ // these members.
+ collectMissingPureVirtuals(Sel);
+
+ if (MissingMethodsByAccess.empty()) {
+ return llvm::make_error<llvm::StringError>(
+ "No pure virtual methods to override.", llvm::inconvertibleErrorCode());
+ }
+
+ const auto &SM = Sel.AST->getSourceManager();
+ const auto &LangOpts = Sel.AST->getLangOpts();
+
+ tooling::Replacements EditReplacements;
+ // Stores text for new access specifier sections // that are not already
+ // present in the class.
+ // Example:
+ // public: // ...
+ // protected: // ...
+ std::string NewSectionsToAppendText;
+ // Tracks if we are adding the first new access section
+ // to NewSectionsToAppendText, to manage preceding newlines.
+ bool IsFirstNewSection = true;
+
+ // Define the order in which access specifiers should be processed and
+ // potentially added.
+ constexpr auto AccessOrder = std::array{
+ AccessSpecifier::AS_public,
+ AccessSpecifier::AS_protected,
+ AccessSpecifier::AS_private,
+ };
+
+ for (AccessSpecifier AS : AccessOrder) {
+ auto GroupIter = MissingMethodsByAccess.find(AS);
+ // Check if there are any missing methods for the current access specifier.
+ if (GroupIter == MissingMethodsByAccess.end() ||
+ GroupIter->second.empty()) {
+ // No methods to override for this access specifier.
+ continue;
+ }
+
+ std::string MethodsGroupString =
+ generateOverridesStringForGroup(GroupIter->second, LangOpts);
+
+ auto ExistingSpecLocIter = AccessSpecifierLocations.find(AS);
+ if (ExistingSpecLocIter != AccessSpecifierLocations.end()) {
+ // Access specifier section already exists in the class.
+ // Get location immediately *after* the colon.
+ SourceLocation InsertLoc =
+ ExistingSpecLocIter->second.getLocWithOffset(1);
+
+ // Create a replacement to insert the method declarations.
+ // The replacement is at InsertLoc, has length 0 (insertion), and uses
+ // InsertionText.
+ std::string InsertionText = "\n" + MethodsGroupString;
+ tooling::Replacement Rep(SM, InsertLoc, 0, InsertionText);
+ if (auto Err = EditReplacements.add(Rep)) {
+ // Handle error if replacement couldn't be added (e.g. overlaps)
+ return llvm::Expected<Tweak::Effect>(std::move(Err));
+ }
+ } else {
+ // Access specifier section does not exist in the class.
+ // These methods will be grouped into NewSectionsToAppendText and added
+ // towards the end of the class definition.
+ if (!IsFirstNewSection) {
+ NewSectionsToAppendText += "\n";
+ }
+ NewSectionsToAppendText +=
+ getAccessSpecifierSpelling(AS) + ":\n" + MethodsGroupString;
+ IsFirstNewSection = false;
+ }
+ }
+
+ // After processing all access specifiers, add any newly created sections
+ // (stored in NewSectionsToAppendText) to the end of the class.
+ if (!NewSectionsToAppendText.empty()) {
+ // AppendLoc is the SourceLocation of the closing brace '}' of the class.
+ // The replacement will insert text *before* this closing brace.
+ SourceLocation AppendLoc = CurrentDecl->getBraceRange().getEnd();
+ std::string FinalAppendText = NewSectionsToAppendText;
+
+ if (!CurrentDecl->decls_empty() || !EditReplacements.empty()) {
+ FinalAppendText = "\n" + FinalAppendText;
+ }
+
+ // Create a replacement to append the new sections.
+ tooling::Replacement Rep(SM, AppendLoc, 0, FinalAppendText);
+ if (auto Err = EditReplacements.add(Rep)) {
+ // Handle error if replacement couldn't be added
+ return llvm::Expected<Tweak::Effect>(std::move(Err));
+ }
+ }
+
+ if (EditReplacements.empty()) {
+ return llvm::make_error<llvm::StringError>(
+ "No changes to apply (internal error or no methods generated).",
+ llvm::inconvertibleErrorCode());
+ }
+
+ // Return the collected replacements as the effect of this tweak.
+ return Effect::mainFileEdit(SM, EditReplacements);
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
diff --git a/clang-tools-extra/clangd/unittests/CMakeLists.txt b/clang-tools-extra/clangd/unittests/CMakeLists.txt
index dffdcd5d014ca..d425070c7f3b7 100644
--- a/clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ b/clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -131,6 +131,7 @@ add_unittest(ClangdUnitTests ClangdTests
tweaks/MemberwiseConstructorTests.cpp
tweaks/ObjCLocalizeStringLiteralTests.cpp
tweaks/ObjCMemberwiseInitializerTests.cpp
+ tweaks/OverridePureVirtualsTests.cpp
tweaks/PopulateSwitchTests.cpp
tweaks/RawStringLiteralTests.cpp
tweaks/RemoveUsingNamespaceTests.cpp
diff --git a/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp
new file mode 100644
index 0000000000000..4b8518cf62209
--- /dev/null
+++ b/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp
@@ -0,0 +1,410 @@
+//===-- AddPureVirtualOverrideTest.cpp --------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "TweakTesting.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+class OverridePureVirtualsTest : public TweakTest {
+protected:
+ OverridePureVirtualsTest() : TweakTest("OverridePureVirtuals") {}
+};
+
+TEST_F(OverridePureVirtualsTest, MinimalUnavailable) {
+ EXPECT_UNAVAILABLE("class ^C {};");
+}
+
+TEST_F(OverridePureVirtualsTest, MinimalAvailable) {
+ EXPECT_AVAILABLE(R"cpp(
+class B { public: virtual void Foo() = 0; };
+class ^C : public B {};
+)cpp");
+}
+
+TEST_F(OverridePureVirtualsTest, Availability) {
+ EXPECT_AVAILABLE(R"cpp(
+class Base {
+public:
+virtual ~Base() = default;
+virtual void F1() = 0;
+virtual void F2() = 0;
+};
+
+class ^Derived : public Base {
+public:
+};
+
+)cpp");
+
+ EXPECT_AVAILABLE(R"cpp(
+class Base {
+public:
+virtual ~Base() = default;
+virtual void F1() = 0;
+virtual void F2() = 0;
+};
+
+class ^Derived : public Base {
+public:
+void F1() override;
+};
+)cpp");
+}
+
+TEST_F(OverridePureVirtualsTest, EmptyDerivedClass) {
+ const char *Before = R"cpp(
+class Base {
+public:
+virtual ~Base() = default;
+virtual void F1() = 0;
+virtual void F2(int P1, const int &P2) = 0;
+};
+
+class ^Derived : public Base {};
+)cpp";
+ const auto *Expected = R"cpp(
+class Base {
+public:
+virtual ~Base() = default;
+virtual void F1() = 0;
+virtual void F2(int P1, const int &P2) = 0;
+};
+
+class Derived : public Base {
+public:
+ void F1() override {
+ // TODO: Implement this pure virtual method
+ static_assert(false, "Method `F1` is not implemented.");
+ }
+ void F2(int P1, const int & P2) override {
+ // TODO: Implement this pure virtual method
+ static_assert(false, "Method `F2` is not implemented.");
+ }
+};
+)cpp";
+ auto Applied = apply(Before);
+ EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
+}
+
+TEST_F(OverridePureVirtualsTest, SingleBaseClassPartiallyImplemented) {
+ auto Applied = apply(
+ R"cpp(
+class Base {
+public:
+virtual ~Base() = default;
+virtual void F1() = 0;
+virtual void F2() = 0;
+};
+
+class ^Derived : public Base {
+public:
+ void F1() override;
+};
+)cpp");
+
+ const auto *Expected = R"cpp(
+class Base {
+public:
+virtual ~Base() = default;
+virtual void F1() = 0;
+virtual void F2() = 0;
+};
+
+class Derived : public Base {
+public:
+ void F2() override {
+ // TODO: Implement this pure virtual method
+ static_assert(false, "Method `F2` is not implemented.");
+ }
+
+ void F1() override;
+};
+)cpp";
+ EXPECT_EQ(Applied, Expected) << "Applied result:\n" << Applied;
+}
+
+TEST_F(OverridePureVirtualsTest, MultipleDirectBaseClasses) {
+ const char *Before = R"cpp(
+class Base1 {
+public:
+ virtual void func1() = 0;
+};
+class Base2 {
+protected:
+ virtual bool func2(char c) const = 0;
+};
+
+class ^Derived : public Base1, public Base2 {};
+)cpp";
+ const auto *Expected = R"cpp(
+class Base1 {
+public:
+ virtual void func1() = 0;
+};
+class Base2 {
+protected:
+ virtual bool func2(char c) const = 0;
+};
+
+class Derived : public Base1, public Base2 {
+public:
+ void func1() override {
+ // TODO: Implement this pure virtual method
+ static_assert(false, "Method `func1` is not implemented.");
+ }
+
+protected:
+ bool func2(char c) const override {
+ // TODO: Implement this pure virtual method
+ static_assert(false, "Method `func2` is not implemented.");
+ }
+};
+)cpp";
+ auto Applied = apply(Before);
+ EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
+}
+
+TEST_F(OverridePureVirtualsTest, UnnamedParametersInBase) {
+ const char *Before = R"cpp(
+struct S {};
+class Base {
+public:
+ virtual void func(int, const S&, char*) = 0;
+};
+
+class ^Derived : public Base {};
+)cpp";
+
+ const auto *Expected = R"cpp(
+struct S {};
+class Base {
+public:
+ virtual void func(int, const S&, char*) = 0;
+};
+
+class Derived : public Base {
+public:
+ void func(int, const S &, char *) override {
+ // TODO: Implement this pure virtual method
+ static_assert(false, "Method `func` is not implemented.");
+ }
+};
+)cpp";
+ auto Applied = apply(Before);
+ EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
+}
+
+TEST_F(OverridePureVirtualsTest, DiamondInheritance) {
+ const char *Before = R"cpp(
+class Top {
+public:
+ virtual ~Top() = default;
+ virtual void diamond_func() = 0;
+};
+class Left : virtual public Top {};
+class Right : virtual public Top {};
+class ^Bottom ...
[truncated]
|
|
@HighCommander4 could you please take a look here, or forward it to anyone who can? |
zyn0217
reviewed
May 10, 2025
clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
Outdated
Show resolved
Hide resolved
zwuis
reviewed
May 10, 2025
There was a problem hiding this comment.
Thank you for your work!
Please add a release note in clang-tools-extra/docs/ReleaseNotes.rst.
Please read https://llvm.org/docs/CodingStandards.html and follow requirements/suggestions in this document such as preferring llvm::SmallVector and not using braces on simple single-statement bodies of if statements.
clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
zwuis
reviewed
May 12, 2025
clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
Outdated
Show resolved
Hide resolved
marcogmaia
commented
May 15, 2025
zwuis
reviewed
May 16, 2025
Apologies, folks, I'm backlogged on reviews and probably won't have time to look at this for a while. If you're looking for a clangd reviewer, perhaps one of our other clangd maintainers, @llvm-beanz or @kadircet, could help with this one. |
marcogmaia
changed the title
Some minor improvements: - Add assert instead of a check that is always false - Add final on tweak class
|
Ping |
1 similar comment
|
Ping |
|
Ping |
|
Ping |
llvm-beanz
approved these changes
Jul 6, 2025
There was a problem hiding this comment.
Sorry for the extreme delay in review. I think everyone trying to keep up with clangd reviews has been swamped and pretty over subscribed.
The implementation here looks clean and follows existing conventions in clangd. It is also quite well commented. The test coverage seems solid.
The one concern I have is that there could be quite a few string allocations and reallocations. Generally I prefer to see code building up strings using SmallString and raw_svector_ostream to build up the string.
That said, much of the code in other clangd tweaks uses std::string pretty aggressively so I think this is probably fine and we can revisit for performance if we encounter issues.
|
Do you need the help to merge this PR? |
kadircet
approved these changes
Jul 9, 2025
There was a problem hiding this comment.
thanks and sorry for not getting back to this for so long. i don't think i'll get to follow up and looks like others are also busy.
i have some usability concerns like:
- Generating method bodies in class definitions instead of out-of-line in implementation files.
- Providing implementations for all missing pure virtuals, without giving user a control might be noisy at times.
- Not providing implementations for some overriden virtuals is also something i'd miss.
But I guess most of these are improvements we can build up-on here. AFAICT, ::prepare is both performance and resilient and we got the docs to figure out intent and fix bugs if need be. So thins LGTM as well.
| @@ -70,6 +70,16 @@ Code completion | |||
| Code actions | |||
| ^^^^^^^^^^^^ | |||
|
|
|||
| - New ``clangd`` code action: "Override pure virtual methods". When invoked on a | |||
There was a problem hiding this comment.
nit: i'd drop clangd, it's already under clangd section.
There was a problem hiding this comment.
Agreed. Dropped.
| - New ``clangd`` code action: "Override pure virtual methods". When invoked on a | ||
| class definition, this action automatically generates C++ ``override`` | ||
| declarations for all pure virtual methods inherited from its base classes that | ||
| have not yet been implemented. The generated method stubs include a ``TODO`` |
There was a problem hiding this comment.
i think content starting with The generated method stubs include a ``TODO`` ... is too much details for release notes, i'd probably drop.
|
Ping |
|
thanks a lot again! |
|
@marcogmaia Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/38756 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/19884 Here is the relevant piece of the build log for the reference |
|
Hi @marcogmaia and @kadircet , I believe the this PR broke a few buildbots. Would you mind taking a look? e.g., The following unit tests were added by this PR UBsan How to reproduce locally: https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild |
qinkunbao
added a commit
that referenced
this pull request
Jul 24, 2025
This reverts commit 7355ea3.
|
Hi, it looks like it still needs some time to fix the build bot errors. I am going to revert this PR according to https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy Please reland this PR after the error is fixed. Thanks! |
qinkunbao
added a commit
that referenced
this pull request
Jul 24, 2025
) This reverts commit 7355ea3. Buildbot failures: UBsan https://lab.llvm.org/buildbot/#/builders/25/builds/10010 Fast https://lab.llvm.org/buildbot/#/builders/169/builds/13150
kadircet
pushed a commit
that referenced
this pull request
Jul 28, 2025
This relands commit 7355ea3. The original commit was reverted in bfd73a5 because it was breaking the buildbot. The issue has now been resolved by 38f8253. Original PR: #139348 Original commit message: <blockquote> closes clangd/clangd#1037 closes clangd/clangd#2240 Example: ```c++ class Base { public: virtual void publicMethod() = 0; protected: virtual auto privateMethod() const -> int = 0; }; // Before: // // cursor here class Derived : public Base{}^ ; // After: class Derived : public Base { public: void publicMethod() override { // TODO: Implement this pure virtual method. static_assert(false, "Method `publicMethod` is not implemented."); } protected: auto privateMethod() const -> int override { // TODO: Implement this pure virtual method. static_assert(false, "Method `privateMethod` is not implemented."); } }; ``` https://github.com/user-attachments/assets/79de40d9-1004-4c2e-8f5c-be1fb074c6de </blockquote>
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jul 28, 2025
…0788) This relands commit llvm/llvm-project@7355ea3. The original commit was reverted in llvm/llvm-project@bfd73a5 because it was breaking the buildbot. The issue has now been resolved by llvm/llvm-project@38f8253. Original PR: llvm/llvm-project#139348 Original commit message: <blockquote> closes clangd/clangd#1037 closes clangd/clangd#2240 Example: ```c++ class Base { public: virtual void publicMethod() = 0; protected: virtual auto privateMethod() const -> int = 0; }; // Before: // // cursor here class Derived : public Base{}^ ; // After: class Derived : public Base { public: void publicMethod() override { // TODO: Implement this pure virtual method. static_assert(false, "Method `publicMethod` is not implemented."); } protected: auto privateMethod() const -> int override { // TODO: Implement this pure virtual method. static_assert(false, "Method `privateMethod` is not implemented."); } }; ``` https://github.com/user-attachments/assets/79de40d9-1004-4c2e-8f5c-be1fb074c6de </blockquote>
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Jul 28, 2025
closes clangd/clangd#1037 closes clangd/clangd#2240 Example: ```c++ class Base { public: virtual void publicMethod() = 0; protected: virtual auto privateMethod() const -> int = 0; }; // Before: // // cursor here class Derived : public Base{}^ ; // After: class Derived : public Base { public: void publicMethod() override { // TODO: Implement this pure virtual method. static_assert(false, "Method `publicMethod` is not implemented."); } protected: auto privateMethod() const -> int override { // TODO: Implement this pure virtual method. static_assert(false, "Method `privateMethod` is not implemented."); } }; ``` https://github.com/user-attachments/assets/79de40d9-1004-4c2e-8f5c-be1fb074c6de --------- Co-authored-by: Marco Maia <marco.maia@iarasystems.com.br>
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Jul 28, 2025
…lvm#150404) This reverts commit 7355ea3. Buildbot failures: UBsan https://lab.llvm.org/buildbot/#/builders/25/builds/10010 Fast https://lab.llvm.org/buildbot/#/builders/169/builds/13150
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes clangd/clangd#1037
closes clangd/clangd#2240
Example:
2025-05-11_16-12-45.mp4