[clang-tools-extra] [clangd] Add tweak to override pure virtuals (PR #139348)
Marco Maia via cfe-commits
cfe-commits at lists.llvm.org
Sun May 18 04:30:57 PDT 2025
https://github.com/marcogmaia updated https://github.com/llvm/llvm-project/pull/139348
>From 76503bd8f5618b710e29333309d1303de5d34723 Mon Sep 17 00:00:00 2001
From: Marco Maia <marco.maia at iarasystems.com.br>
Date: Sat, 10 May 2025 00:48:39 -0300
Subject: [PATCH 01/12] [clangd] Add tweak to add pure virtual overrides
---
.../clangd/refactor/tweaks/CMakeLists.txt | 3 +-
.../refactor/tweaks/OverridePureVirtuals.cpp | 366 ++++++++++++++++
.../clangd/unittests/CMakeLists.txt | 1 +
.../tweaks/OverridePureVirtualsTests.cpp | 410 ++++++++++++++++++
4 files changed, 779 insertions(+), 1 deletion(-)
create mode 100644 clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
create mode 100644 clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp
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 : public Left, public Right {};
+)cpp";
+ const auto *Expected = 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 : public Left, public Right {
+public:
+ void diamond_func() override {
+ // TODO: Implement this pure virtual method
+ static_assert(false, "Method `diamond_func` is not implemented.");
+ }
+};
+)cpp";
+ auto Applied = apply(Before);
+ EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
+}
+
+TEST_F(OverridePureVirtualsTest, MixedAccessSpecifiers) {
+ const char *Before = R"cpp(
+class Base {
+public:
+ virtual void pub_func() = 0;
+ virtual void pub_func2(char) const = 0;
+protected:
+ virtual int prot_func(int x) const = 0;
+};
+
+class ^Derived : public Base {
+ int member; // Existing member
+public:
+ Derived(int m) : member(m) {}
+};
+)cpp";
+ const auto *Expected = R"cpp(
+class Base {
+public:
+ virtual void pub_func() = 0;
+ virtual void pub_func2(char) const = 0;
+protected:
+ virtual int prot_func(int x) const = 0;
+};
+
+class Derived : public Base {
+ int member; // Existing member
+public:
+ void pub_func() override {
+ // TODO: Implement this pure virtual method
+ static_assert(false, "Method `pub_func` is not implemented.");
+ }
+ void pub_func2(char) const override {
+ // TODO: Implement this pure virtual method
+ static_assert(false, "Method `pub_func2` is not implemented.");
+ }
+
+ Derived(int m) : member(m) {}
+
+protected:
+ int prot_func(int x) const override {
+ // TODO: Implement this pure virtual method
+ static_assert(false, "Method `prot_func` is not implemented.");
+ }
+};
+)cpp";
+ auto Applied = apply(Before);
+ EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
+}
+
+TEST_F(OverridePureVirtualsTest, OutOfOrderMixedAccessSpecifiers) {
+ const char *Before = R"cpp(
+class Base {
+public:
+ virtual void pub_func() = 0;
+ virtual void pub_func2(char) const = 0;
+protected:
+ virtual int prot_func(int x) const = 0;
+};
+
+class ^Derived : public Base {
+ int member; // Existing member
+protected:
+ void foo();
+public:
+ Derived(int m) : member(m) {}
+};
+)cpp";
+ const auto *Expected = R"cpp(
+class Base {
+public:
+ virtual void pub_func() = 0;
+ virtual void pub_func2(char) const = 0;
+protected:
+ virtual int prot_func(int x) const = 0;
+};
+
+class Derived : public Base {
+ int member; // Existing member
+protected:
+ int prot_func(int x) const override {
+ // TODO: Implement this pure virtual method
+ static_assert(false, "Method `prot_func` is not implemented.");
+ }
+
+ void foo();
+public:
+ void pub_func() override {
+ // TODO: Implement this pure virtual method
+ static_assert(false, "Method `pub_func` is not implemented.");
+ }
+ void pub_func2(char) const override {
+ // TODO: Implement this pure virtual method
+ static_assert(false, "Method `pub_func2` is not implemented.");
+ }
+
+ Derived(int m) : member(m) {}
+};
+)cpp";
+ auto Applied = apply(Before);
+ EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
+}
+
+TEST_F(OverridePureVirtualsTest, MultiAccessSpecifiersOverride) {
+ constexpr auto Before = R"cpp(
+class Base {
+public:
+ virtual void foo() = 0;
+protected:
+ virtual void bar() = 0;
+};
+
+class ^Derived : public Base {};
+)cpp";
+
+ constexpr auto Expected = R"cpp(
+class Base {
+public:
+ virtual void foo() = 0;
+protected:
+ virtual void bar() = 0;
+};
+
+class Derived : public Base {
+public:
+ void foo() override {
+ // TODO: Implement this pure virtual method
+ static_assert(false, "Method `foo` is not implemented.");
+ }
+
+protected:
+ void bar() override {
+ // TODO: Implement this pure virtual method
+ static_assert(false, "Method `bar` is not implemented.");
+ }
+};
+)cpp";
+ auto Applied = apply(Before);
+ EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
+}
+
+TEST_F(OverridePureVirtualsTest, AccessSpecifierAlreadyExisting) {
+ const char *Before = R"cpp(
+class Base {
+public:
+ virtual void func1() = 0;
+};
+
+class ^Derived : public Base {
+public:
+};
+)cpp";
+
+ const auto *Expected = R"cpp(
+class Base {
+public:
+ virtual void func1() = 0;
+};
+
+class Derived : public Base {
+public:
+ void func1() override {
+ // TODO: Implement this pure virtual method
+ static_assert(false, "Method `func1` is not implemented.");
+ }
+
+};
+)cpp";
+ auto Applied = apply(Before);
+ EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
>From aadebcbe2508e2cb6912f443b331bb1d95984066 Mon Sep 17 00:00:00 2001
From: Marco Maia <marcogmaia at gmail.com>
Date: Sat, 10 May 2025 08:57:45 -0300
Subject: [PATCH 02/12] Replace `getAccessSpecifierSpelling` with
`getAccessSpelling`
---
.../refactor/tweaks/OverridePureVirtuals.cpp | 19 +------------------
1 file changed, 1 insertion(+), 18 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
index b8880433fdd52..82e43d8f92017 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
@@ -241,23 +241,6 @@ generateOverridesStringForGroup(std::vector<const CXXMethodDecl *> Methods,
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.
@@ -326,7 +309,7 @@ Expected<Tweak::Effect> OverridePureVirtuals::apply(const Selection &Sel) {
NewSectionsToAppendText += "\n";
}
NewSectionsToAppendText +=
- getAccessSpecifierSpelling(AS) + ":\n" + MethodsGroupString;
+ getAccessSpelling(AS).str() + ":\n" + MethodsGroupString;
IsFirstNewSection = false;
}
}
>From f8a40a7ef4ae9bc74f00ebb95b1373b5068f44e4 Mon Sep 17 00:00:00 2001
From: Marco Maia <marcogmaia at gmail.com>
Date: Sat, 10 May 2025 18:44:18 -0300
Subject: [PATCH 03/12] Refactor code and remove const
---
.../refactor/tweaks/OverridePureVirtuals.cpp | 167 +++++++-----------
1 file changed, 60 insertions(+), 107 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
index 82e43d8f92017..8e6ef112ba1e0 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
@@ -18,9 +18,6 @@
#include "llvm/ADT/DenseSet.h"
#include "llvm/Support/FormatVariadic.h"
#include <algorithm>
-#include <functional>
-#include <map>
-#include <set>
#include <string>
#include <vector>
@@ -35,18 +32,18 @@ class OverridePureVirtuals : public Tweak {
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;
+ return CodeAction::QUICKFIX_KIND;
}
private:
// Stores the CXXRecordDecl of the class being modified.
- const CXXRecordDecl *CurrentDecl = nullptr;
+ const CXXRecordDecl *CurrentDeclDef = nullptr;
// Stores pure virtual methods that need overriding, grouped by their original
// access specifier.
- std::map<AccessSpecifier, std::vector<const CXXMethodDecl *>>
+ llvm::MapVector<AccessSpecifier, std::vector<const CXXMethodDecl *>>
MissingMethodsByAccess;
// Stores the source locations of existing access specifiers in CurrentDecl.
- std::map<AccessSpecifier, SourceLocation> AccessSpecifierLocations;
+ llvm::MapVector<AccessSpecifier, SourceLocation> AccessSpecifierLocations;
// Helper function to gather information before applying the tweak.
void collectMissingPureVirtuals(const Selection &Sel);
@@ -54,69 +51,52 @@ class OverridePureVirtuals : public Tweak {
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;
- }
+// Function to get all unique pure virtual methods from the entire
+// base class hierarchy of CurrentDeclDef.
+llvm::SmallVector<const clang::CXXMethodDecl *>
+getAllUniquePureVirtualsFromBaseHierarchy(
+ const clang::CXXRecordDecl *CurrentDeclDef) {
+ llvm::SmallVector<const clang::CXXMethodDecl *> AllPureVirtualsInHierarchy;
+ llvm::DenseSet<const clang::CXXMethodDecl *> CanonicalPureVirtualsSeen;
- 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.
- }
- }
+ if (!CurrentDeclDef || !CurrentDeclDef->getDefinition()) {
+ return AllPureVirtualsInHierarchy;
+ }
+ const clang::CXXRecordDecl *Def = CurrentDeclDef->getDefinition();
- for (const auto &BaseSpec : Def->bases()) {
- if (const CXXRecordDecl *BaseDef =
- BaseSpec.getType()->getAsCXXRecordDecl()) {
- Collect(BaseDef); // Recursively collect from base classes.
+ Def->forallBases([&](const clang::CXXRecordDecl *BaseDefinition) {
+ for (const clang::CXXMethodDecl *Method : BaseDefinition->methods()) {
+ if (Method->isPureVirtual() &&
+ CanonicalPureVirtualsSeen.insert(Method->getCanonicalDecl()).second) {
+ AllPureVirtualsInHierarchy.emplace_back(Method);
}
}
- };
+ return true; // Continue iterating through all bases
+ });
- Collect(Decl);
- return Result;
+ return AllPureVirtualsInHierarchy;
}
// Gets canonical declarations of methods already overridden or implemented in
// class D.
-std::set<const CXXMethodDecl *>
+llvm::SetVector<const CXXMethodDecl *>
getImplementedOrOverriddenCanonicals(const CXXRecordDecl *D) {
- std::set<const CXXMethodDecl *> ImplementedSet;
+ llvm::SetVector<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()) {
+ 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>
+llvm::MapVector<AccessSpecifier, SourceLocation>
getSpecifierLocations(const CXXRecordDecl *D) {
- std::map<AccessSpecifier, SourceLocation> Locs;
- for (auto *DeclNode : D->decls()) { // Changed to DeclNode to avoid ambiguity
+ llvm::MapVector<AccessSpecifier, SourceLocation> Locs;
+ for (auto *DeclNode : D->decls()) {
if (const auto *ASD = llvm::dyn_cast<AccessSpecDecl>(DeclNode)) {
Locs[ASD->getAccess()] = ASD->getColonLoc();
}
@@ -124,85 +104,58 @@ getSpecifierLocations(const CXXRecordDecl *D) {
return Locs;
}
+bool hasAbstractBaseAncestor(const clang::CXXRecordDecl *CurrentDecl) {
+ if (!CurrentDecl || !CurrentDecl->getDefinition())
+ return false;
+
+ return CurrentDecl->getDefinition()->forallBases(
+ [](const clang::CXXRecordDecl *BaseDefinition) {
+ return BaseDefinition->isAbstract();
+ });
+}
+
// 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) {
+ if (!Node)
return false;
- }
// Make sure we have a definition.
- CurrentDecl = Node->ASTNode.get<CXXRecordDecl>();
- if (!CurrentDecl || !CurrentDecl->getDefinition()) {
+ CurrentDeclDef = Node->ASTNode.get<CXXRecordDecl>();
+ if (!CurrentDeclDef || !CurrentDeclDef->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;
- }
- }
- }
+ // From now on, we should work with the definition.
+ CurrentDeclDef = CurrentDeclDef->getDefinition();
// Only offer for polymorphic classes with abstract bases.
- return CurrentDecl->isPolymorphic() && HasAbstractBase;
+ return CurrentDeclDef->isPolymorphic() &&
+ hasAbstractBaseAncestor(CurrentDeclDef);
}
// 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)
+ if (!CurrentDeclDef)
return;
- AccessSpecifierLocations = getSpecifierLocations(CurrentDecl);
+ AccessSpecifierLocations = getSpecifierLocations(CurrentDeclDef);
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);
- }
- }
- }
- }
+ llvm::SmallVector<const CXXMethodDecl *> AllPureVirtualsInHierarchy =
+ getAllUniquePureVirtualsFromBaseHierarchy(CurrentDeclDef);
// Get methods already implemented or overridden in CurrentDecl.
const auto ImplementedOrOverriddenSet =
- getImplementedOrOverriddenCanonicals(CurrentDecl);
+ getImplementedOrOverriddenCanonicals(CurrentDeclDef);
// Filter AllPureVirtualsInHierarchy to find those not in
- // ImplementedOrOverriddenSet.
+ // ImplementedOrOverriddenSet, which needs to be overriden.
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.
+ bool AlreadyHandled = ImplementedOrOverriddenSet.contains(BaseMethod);
+ if (!AlreadyHandled)
MissingMethodsByAccess[BaseMethod->getAccess()].emplace_back(BaseMethod);
- }
}
}
@@ -221,11 +174,11 @@ generateOverridesStringForGroup(std::vector<const CXXMethodDecl *> Methods,
std::string MethodsString;
for (const auto *Method : Methods) {
- std::vector<std::string> ParamsAsString;
+ llvm::SmallVector<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, ", ");
+ auto Params = llvm::join(ParamsAsString, ", ");
MethodsString +=
llvm::formatv(
@@ -274,7 +227,7 @@ Expected<Tweak::Effect> OverridePureVirtuals::apply(const Selection &Sel) {
};
for (AccessSpecifier AS : AccessOrder) {
- auto GroupIter = MissingMethodsByAccess.find(AS);
+ auto *GroupIter = MissingMethodsByAccess.find(AS);
// Check if there are any missing methods for the current access specifier.
if (GroupIter == MissingMethodsByAccess.end() ||
GroupIter->second.empty()) {
@@ -285,7 +238,7 @@ Expected<Tweak::Effect> OverridePureVirtuals::apply(const Selection &Sel) {
std::string MethodsGroupString =
generateOverridesStringForGroup(GroupIter->second, LangOpts);
- auto ExistingSpecLocIter = AccessSpecifierLocations.find(AS);
+ auto *ExistingSpecLocIter = AccessSpecifierLocations.find(AS);
if (ExistingSpecLocIter != AccessSpecifierLocations.end()) {
// Access specifier section already exists in the class.
// Get location immediately *after* the colon.
@@ -319,10 +272,10 @@ Expected<Tweak::Effect> OverridePureVirtuals::apply(const Selection &Sel) {
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();
+ SourceLocation AppendLoc = CurrentDeclDef->getBraceRange().getEnd();
std::string FinalAppendText = NewSectionsToAppendText;
- if (!CurrentDecl->decls_empty() || !EditReplacements.empty()) {
+ if (!CurrentDeclDef->decls_empty() || !EditReplacements.empty()) {
FinalAppendText = "\n" + FinalAppendText;
}
>From 84f127bda679f9c44e5d0cec95a04a8faa45f371 Mon Sep 17 00:00:00 2001
From: Marco Maia <marcogmaia at gmail.com>
Date: Sat, 10 May 2025 20:22:56 -0300
Subject: [PATCH 04/12] Make use of llvm::any_of and more move operations
---
.../refactor/tweaks/OverridePureVirtuals.cpp | 28 +++++++++----------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
index 8e6ef112ba1e0..f9862a74007ed 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
@@ -17,7 +17,6 @@
#include "clang/Tooling/Core/Replacement.h"
#include "llvm/ADT/DenseSet.h"
#include "llvm/Support/FormatVariadic.h"
-#include <algorithm>
#include <string>
#include <vector>
@@ -108,9 +107,11 @@ bool hasAbstractBaseAncestor(const clang::CXXRecordDecl *CurrentDecl) {
if (!CurrentDecl || !CurrentDecl->getDefinition())
return false;
- return CurrentDecl->getDefinition()->forallBases(
- [](const clang::CXXRecordDecl *BaseDefinition) {
- return BaseDefinition->isAbstract();
+ return llvm::any_of(
+ CurrentDecl->getDefinition()->bases(), [](CXXBaseSpecifier BaseSpec) {
+ const auto *D = BaseSpec.getType()->getAsCXXRecordDecl();
+ const auto *Def = D ? D->getDefinition() : nullptr;
+ return Def && Def->isAbstract();
});
}
@@ -169,15 +170,16 @@ generateOverridesStringForGroup(std::vector<const CXXMethodDecl *> Methods,
// Unnamed parameter.
return TypeStr;
}
- return llvm::formatv("{0} {1}", TypeStr, P->getNameAsString()).str();
+ return llvm::formatv("{0} {1}", std::move(TypeStr), P->getNameAsString())
+ .str();
};
std::string MethodsString;
for (const auto *Method : Methods) {
llvm::SmallVector<std::string> ParamsAsString;
ParamsAsString.reserve(Method->parameters().size());
- std::transform(Method->param_begin(), Method->param_end(),
- std::back_inserter(ParamsAsString), GetParamString);
+ llvm::transform(Method->parameters(), std::back_inserter(ParamsAsString),
+ GetParamString);
auto Params = llvm::join(ParamsAsString, ", ");
MethodsString +=
@@ -187,7 +189,7 @@ generateOverridesStringForGroup(std::vector<const CXXMethodDecl *> Methods,
" static_assert(false, \"Method `{1}` is not implemented.\");\n"
" }\n",
Method->getReturnType().getAsString(LangOpts),
- Method->getNameAsString(), Params,
+ Method->getNameAsString(), std::move(Params),
std::string(Method->isConst() ? "const " : ""))
.str();
}
@@ -229,11 +231,9 @@ Expected<Tweak::Effect> OverridePureVirtuals::apply(const Selection &Sel) {
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.
+ // No methods to override for this access specifier.
+ if (GroupIter == MissingMethodsByAccess.end() || GroupIter->second.empty())
continue;
- }
std::string MethodsGroupString =
generateOverridesStringForGroup(GroupIter->second, LangOpts);
@@ -281,10 +281,8 @@ Expected<Tweak::Effect> OverridePureVirtuals::apply(const Selection &Sel) {
// 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
+ if (auto Err = EditReplacements.add(Rep))
return llvm::Expected<Tweak::Effect>(std::move(Err));
- }
}
if (EditReplacements.empty()) {
>From 8f8ed30e603ea6ac9e2b4ac4e9e8d98fbc86a4fa Mon Sep 17 00:00:00 2001
From: Marco Maia <marcogmaia at gmail.com>
Date: Sat, 10 May 2025 20:37:01 -0300
Subject: [PATCH 05/12] Update `clang-tools-extra` release notes
---
clang-tools-extra/docs/ReleaseNotes.rst | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 579fca54924d5..d4b1d534f27c8 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -70,6 +70,16 @@ Code completion
Code actions
^^^^^^^^^^^^
+- 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``
+ comment and a ``static_assert(false, "Method 'method_name' is not
+ implemented.")`` to prompt the user for the actual implementation. The
+ overrides are intelligently grouped under their original access specifiers
+ (e.g., ``public``, ``protected``), creating new access specifier blocks if
+ necessary.
+
Signature help
^^^^^^^^^^^^^^
>From 1bfc24f60e69f224d1b26aac7818a30ee548159d Mon Sep 17 00:00:00 2001
From: Marco Maia <marcogmaia at gmail.com>
Date: Sat, 10 May 2025 21:34:57 -0300
Subject: [PATCH 06/12] Simplify if/for statements
---
.../refactor/tweaks/OverridePureVirtuals.cpp | 24 ++++++++-----------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
index f9862a74007ed..4fc3eee26c923 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
@@ -58,17 +58,16 @@ getAllUniquePureVirtualsFromBaseHierarchy(
llvm::SmallVector<const clang::CXXMethodDecl *> AllPureVirtualsInHierarchy;
llvm::DenseSet<const clang::CXXMethodDecl *> CanonicalPureVirtualsSeen;
- if (!CurrentDeclDef || !CurrentDeclDef->getDefinition()) {
+ if (!CurrentDeclDef || !CurrentDeclDef->getDefinition())
return AllPureVirtualsInHierarchy;
- }
+
const clang::CXXRecordDecl *Def = CurrentDeclDef->getDefinition();
Def->forallBases([&](const clang::CXXRecordDecl *BaseDefinition) {
for (const clang::CXXMethodDecl *Method : BaseDefinition->methods()) {
if (Method->isPureVirtual() &&
- CanonicalPureVirtualsSeen.insert(Method->getCanonicalDecl()).second) {
+ CanonicalPureVirtualsSeen.insert(Method->getCanonicalDecl()).second)
AllPureVirtualsInHierarchy.emplace_back(Method);
- }
}
return true; // Continue iterating through all bases
});
@@ -96,9 +95,8 @@ llvm::MapVector<AccessSpecifier, SourceLocation>
getSpecifierLocations(const CXXRecordDecl *D) {
llvm::MapVector<AccessSpecifier, SourceLocation> Locs;
for (auto *DeclNode : D->decls()) {
- if (const auto *ASD = llvm::dyn_cast<AccessSpecDecl>(DeclNode)) {
+ if (const auto *ASD = llvm::dyn_cast<AccessSpecDecl>(DeclNode))
Locs[ASD->getAccess()] = ASD->getColonLoc();
- }
}
return Locs;
}
@@ -166,10 +164,10 @@ 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.
+ // Unnamed parameter.
+ if (P->getNameAsString().empty())
return TypeStr;
- }
+
return llvm::formatv("{0} {1}", std::move(TypeStr), P->getNameAsString())
.str();
};
@@ -250,17 +248,15 @@ Expected<Tweak::Effect> OverridePureVirtuals::apply(const Selection &Sel) {
// 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)
+ if (auto Err = EditReplacements.add(Rep))
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) {
+ if (!IsFirstNewSection)
NewSectionsToAppendText += "\n";
- }
+
NewSectionsToAppendText +=
getAccessSpelling(AS).str() + ":\n" + MethodsGroupString;
IsFirstNewSection = false;
>From 248157175e05882806dd980b1523bb552e3deaf1 Mon Sep 17 00:00:00 2001
From: Marco Maia <marcogmaia at gmail.com>
Date: Sat, 10 May 2025 21:39:22 -0300
Subject: [PATCH 07/12] Replace std::vector in favor of llvm::SmallVector
---
.../clangd/refactor/tweaks/OverridePureVirtuals.cpp | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
index 4fc3eee26c923..0a1a873b5bfbb 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
@@ -18,7 +18,6 @@
#include "llvm/ADT/DenseSet.h"
#include "llvm/Support/FormatVariadic.h"
#include <string>
-#include <vector>
namespace clang {
namespace clangd {
@@ -39,7 +38,7 @@ class OverridePureVirtuals : public Tweak {
const CXXRecordDecl *CurrentDeclDef = nullptr;
// Stores pure virtual methods that need overriding, grouped by their original
// access specifier.
- llvm::MapVector<AccessSpecifier, std::vector<const CXXMethodDecl *>>
+ llvm::MapVector<AccessSpecifier, llvm::SmallVector<const CXXMethodDecl *>>
MissingMethodsByAccess;
// Stores the source locations of existing access specifiers in CurrentDecl.
llvm::MapVector<AccessSpecifier, SourceLocation> AccessSpecifierLocations;
@@ -159,9 +158,9 @@ void OverridePureVirtuals::collectMissingPureVirtuals(const Selection &Sel) {
}
// Free function to generate the string for a group of method overrides.
-std::string
-generateOverridesStringForGroup(std::vector<const CXXMethodDecl *> Methods,
- const LangOptions &LangOpts) {
+std::string generateOverridesStringForGroup(
+ llvm::SmallVector<const CXXMethodDecl *> Methods,
+ const LangOptions &LangOpts) {
const auto GetParamString = [&LangOpts](const ParmVarDecl *P) {
std::string TypeStr = P->getType().getAsString(LangOpts);
// Unnamed parameter.
>From 04faa13443413470531cb867dfe1f044cf373627 Mon Sep 17 00:00:00 2001
From: Marco Maia <marcogmaia at gmail.com>
Date: Sun, 11 May 2025 03:33:22 -0300
Subject: [PATCH 08/12] Make code action unavailable when already overridden
---
.../refactor/tweaks/OverridePureVirtuals.cpp | 4 ++--
.../tweaks/OverridePureVirtualsTests.cpp | 15 +++++++++++++++
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
index 0a1a873b5bfbb..df74b27b143bb 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
@@ -126,8 +126,8 @@ bool OverridePureVirtuals::prepare(const Selection &Sel) {
// From now on, we should work with the definition.
CurrentDeclDef = CurrentDeclDef->getDefinition();
- // Only offer for polymorphic classes with abstract bases.
- return CurrentDeclDef->isPolymorphic() &&
+ // Only offer for abstract classes with abstract bases.
+ return CurrentDeclDef->isAbstract() &&
hasAbstractBaseAncestor(CurrentDeclDef);
}
diff --git a/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp
index 4b8518cf62209..22a9ca079a7e4 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp
@@ -29,6 +29,21 @@ class ^C : public B {};
)cpp");
}
+TEST_F(OverridePureVirtualsTest, UnavailableWhenOverriden) {
+ EXPECT_UNAVAILABLE(
+ R"cpp(
+class B {
+public:
+ virtual void foo() = 0;
+};
+
+class ^D : public B {
+public:
+ void foo() override;
+};
+)cpp");
+}
+
TEST_F(OverridePureVirtualsTest, Availability) {
EXPECT_AVAILABLE(R"cpp(
class Base {
>From 7125ddb6da12925e2aced693859b1e7cf441140e Mon Sep 17 00:00:00 2001
From: Marco Maia <marcogmaia at gmail.com>
Date: Sun, 11 May 2025 20:01:34 -0300
Subject: [PATCH 09/12] Minor spelling fixes
---
.../refactor/tweaks/OverridePureVirtuals.cpp | 9 +++---
.../tweaks/OverridePureVirtualsTests.cpp | 32 +++++++++----------
2 files changed, 21 insertions(+), 20 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
index df74b27b143bb..210fba2721863 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
@@ -1,4 +1,4 @@
-//===--- AddPureVirtualOverride.cpp ------------------------------*- C++-*-===//
+//===--- OverridePureVirtuals.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.
@@ -68,7 +68,8 @@ getAllUniquePureVirtualsFromBaseHierarchy(
CanonicalPureVirtualsSeen.insert(Method->getCanonicalDecl()).second)
AllPureVirtualsInHierarchy.emplace_back(Method);
}
- return true; // Continue iterating through all bases
+ // Continue iterating through all bases.
+ return true;
});
return AllPureVirtualsInHierarchy;
@@ -207,8 +208,8 @@ Expected<Tweak::Effect> OverridePureVirtuals::apply(const Selection &Sel) {
const auto &LangOpts = Sel.AST->getLangOpts();
tooling::Replacements EditReplacements;
- // Stores text for new access specifier sections // that are not already
- // present in the class.
+ // Stores text for new access specifier sections that are not already present
+ // in the class.
// Example:
// public: // ...
// protected: // ...
diff --git a/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp
index 22a9ca079a7e4..83910221e1af3 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp
@@ -1,4 +1,4 @@
-//===-- AddPureVirtualOverrideTest.cpp --------------------------*- C++ -*-===//
+//===-- OverridePureVirtualsTests.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.
@@ -13,23 +13,23 @@ namespace clang {
namespace clangd {
namespace {
-class OverridePureVirtualsTest : public TweakTest {
+class OverridePureVirtualsTests : public TweakTest {
protected:
- OverridePureVirtualsTest() : TweakTest("OverridePureVirtuals") {}
+ OverridePureVirtualsTests() : TweakTest("OverridePureVirtuals") {}
};
-TEST_F(OverridePureVirtualsTest, MinimalUnavailable) {
+TEST_F(OverridePureVirtualsTests, MinimalUnavailable) {
EXPECT_UNAVAILABLE("class ^C {};");
}
-TEST_F(OverridePureVirtualsTest, MinimalAvailable) {
+TEST_F(OverridePureVirtualsTests, MinimalAvailable) {
EXPECT_AVAILABLE(R"cpp(
class B { public: virtual void Foo() = 0; };
class ^C : public B {};
)cpp");
}
-TEST_F(OverridePureVirtualsTest, UnavailableWhenOverriden) {
+TEST_F(OverridePureVirtualsTests, UnavailableWhenOverriden) {
EXPECT_UNAVAILABLE(
R"cpp(
class B {
@@ -44,7 +44,7 @@ class ^D : public B {
)cpp");
}
-TEST_F(OverridePureVirtualsTest, Availability) {
+TEST_F(OverridePureVirtualsTests, Availability) {
EXPECT_AVAILABLE(R"cpp(
class Base {
public:
@@ -74,7 +74,7 @@ void F1() override;
)cpp");
}
-TEST_F(OverridePureVirtualsTest, EmptyDerivedClass) {
+TEST_F(OverridePureVirtualsTests, EmptyDerivedClass) {
const char *Before = R"cpp(
class Base {
public:
@@ -109,7 +109,7 @@ class Derived : public Base {
EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
}
-TEST_F(OverridePureVirtualsTest, SingleBaseClassPartiallyImplemented) {
+TEST_F(OverridePureVirtualsTests, SingleBaseClassPartiallyImplemented) {
auto Applied = apply(
R"cpp(
class Base {
@@ -146,7 +146,7 @@ class Derived : public Base {
EXPECT_EQ(Applied, Expected) << "Applied result:\n" << Applied;
}
-TEST_F(OverridePureVirtualsTest, MultipleDirectBaseClasses) {
+TEST_F(OverridePureVirtualsTests, MultipleDirectBaseClasses) {
const char *Before = R"cpp(
class Base1 {
public:
@@ -187,7 +187,7 @@ class Derived : public Base1, public Base2 {
EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
}
-TEST_F(OverridePureVirtualsTest, UnnamedParametersInBase) {
+TEST_F(OverridePureVirtualsTests, UnnamedParametersInBase) {
const char *Before = R"cpp(
struct S {};
class Base {
@@ -217,7 +217,7 @@ class Derived : public Base {
EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
}
-TEST_F(OverridePureVirtualsTest, DiamondInheritance) {
+TEST_F(OverridePureVirtualsTests, DiamondInheritance) {
const char *Before = R"cpp(
class Top {
public:
@@ -248,7 +248,7 @@ class Bottom : public Left, public Right {
EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
}
-TEST_F(OverridePureVirtualsTest, MixedAccessSpecifiers) {
+TEST_F(OverridePureVirtualsTests, MixedAccessSpecifiers) {
const char *Before = R"cpp(
class Base {
public:
@@ -298,7 +298,7 @@ class Derived : public Base {
EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
}
-TEST_F(OverridePureVirtualsTest, OutOfOrderMixedAccessSpecifiers) {
+TEST_F(OverridePureVirtualsTests, OutOfOrderMixedAccessSpecifiers) {
const char *Before = R"cpp(
class Base {
public:
@@ -351,7 +351,7 @@ class Derived : public Base {
EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
}
-TEST_F(OverridePureVirtualsTest, MultiAccessSpecifiersOverride) {
+TEST_F(OverridePureVirtualsTests, MultiAccessSpecifiersOverride) {
constexpr auto Before = R"cpp(
class Base {
public:
@@ -389,7 +389,7 @@ class Derived : public Base {
EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
}
-TEST_F(OverridePureVirtualsTest, AccessSpecifierAlreadyExisting) {
+TEST_F(OverridePureVirtualsTests, AccessSpecifierAlreadyExisting) {
const char *Before = R"cpp(
class Base {
public:
>From 8e73db3270d8cb5bb134c91260ea48ea7d081485 Mon Sep 17 00:00:00 2001
From: Marco Maia <marcogmaia at gmail.com>
Date: Wed, 14 May 2025 23:03:37 -0300
Subject: [PATCH 10/12] Apply @zwuis review
Addressing unusual cases of method declaration.
- constexpr/consteval specifier
- Ref-qualifier
- trailing return type
Refactored the logic for the method definition grouping
---
.../refactor/tweaks/OverridePureVirtuals.cpp | 132 +++----
.../tweaks/OverridePureVirtualsTests.cpp | 333 +++++++++++++++++-
2 files changed, 385 insertions(+), 80 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
index 210fba2721863..72343af5b8a29 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
@@ -8,6 +8,8 @@
#include "refactor/Tweak.h"
+#include "support/Token.h"
+
#include "clang/AST/ASTContext.h"
#include "clang/AST/DeclCXX.h"
#include "clang/AST/Type.h"
@@ -23,6 +25,40 @@ namespace clang {
namespace clangd {
namespace {
+// This function removes the "virtual" and the "= 0" at the end;
+// e.g.:
+// "virtual void foo(int var = 0) = 0" // input.
+// "void foo(int var = 0)" // output.
+std::string removePureVirtualSyntax(const std::string &MethodDecl,
+ const LangOptions &LangOpts) {
+ assert(!MethodDecl.empty());
+
+ TokenStream TS = lex(MethodDecl, LangOpts);
+
+ std::string DeclString;
+ for (const clangd::Token &Tk : TS.tokens()) {
+ if (Tk.Kind == clang::tok::raw_identifier && Tk.text() == "virtual")
+ continue;
+
+ // If the ending two tokens are "= 0", we break here and we already have the
+ // method's string without the pure virtual syntax.
+ const auto &Next = Tk.next();
+ if (Next.next().Kind == tok::eof && Tk.Kind == clang::tok::equal &&
+ Next.text() == "0")
+ break;
+
+ DeclString += Tk.text();
+ if (Tk.Kind != tok::l_paren && Next.Kind != tok::comma &&
+ Next.Kind != tok::r_paren && Next.Kind != tok::l_paren)
+ DeclString += ' ';
+ }
+ // Trim the last whitespace.
+ if (DeclString.back() == ' ')
+ DeclString.pop_back();
+
+ return DeclString;
+}
+
class OverridePureVirtuals : public Tweak {
public:
const char *id() const final; // defined by REGISTER_TWEAK.
@@ -44,7 +80,7 @@ class OverridePureVirtuals : public Tweak {
llvm::MapVector<AccessSpecifier, SourceLocation> AccessSpecifierLocations;
// Helper function to gather information before applying the tweak.
- void collectMissingPureVirtuals(const Selection &Sel);
+ void collectMissingPureVirtuals();
};
REGISTER_TWEAK(OverridePureVirtuals)
@@ -102,8 +138,7 @@ getSpecifierLocations(const CXXRecordDecl *D) {
}
bool hasAbstractBaseAncestor(const clang::CXXRecordDecl *CurrentDecl) {
- if (!CurrentDecl || !CurrentDecl->getDefinition())
- return false;
+ assert(CurrentDecl && CurrentDecl->getDefinition());
return llvm::any_of(
CurrentDecl->getDefinition()->bases(), [](CXXBaseSpecifier BaseSpec) {
@@ -134,7 +169,7 @@ bool OverridePureVirtuals::prepare(const Selection &Sel) {
// Collects all pure virtual methods that are missing an override in
// CurrentDecl, grouped by their original access specifier.
-void OverridePureVirtuals::collectMissingPureVirtuals(const Selection &Sel) {
+void OverridePureVirtuals::collectMissingPureVirtuals() {
if (!CurrentDeclDef)
return;
@@ -158,46 +193,39 @@ void OverridePureVirtuals::collectMissingPureVirtuals(const Selection &Sel) {
}
}
+std::string generateOverrideString(const CXXMethodDecl *Method,
+ const LangOptions &LangOpts) {
+ std::string MethodDecl;
+ auto OS = llvm::raw_string_ostream(MethodDecl);
+ Method->print(OS);
+
+ return llvm::formatv(
+ "\n {0} override {{\n"
+ " // TODO: Implement this pure virtual method.\n"
+ " static_assert(false, \"Method `{1}` is not implemented.\");\n"
+ " }",
+ removePureVirtualSyntax(MethodDecl, LangOpts), Method->getName())
+ .str();
+}
+
// Free function to generate the string for a group of method overrides.
std::string generateOverridesStringForGroup(
llvm::SmallVector<const CXXMethodDecl *> Methods,
const LangOptions &LangOpts) {
- const auto GetParamString = [&LangOpts](const ParmVarDecl *P) {
- std::string TypeStr = P->getType().getAsString(LangOpts);
- // Unnamed parameter.
- if (P->getNameAsString().empty())
- return TypeStr;
-
- return llvm::formatv("{0} {1}", std::move(TypeStr), P->getNameAsString())
- .str();
- };
-
- std::string MethodsString;
+ llvm::SmallVector<std::string> MethodsString;
+ MethodsString.reserve(Methods.size());
+ std::string SS;
for (const auto *Method : Methods) {
- llvm::SmallVector<std::string> ParamsAsString;
- ParamsAsString.reserve(Method->parameters().size());
- llvm::transform(Method->parameters(), std::back_inserter(ParamsAsString),
- GetParamString);
- 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(), std::move(Params),
- std::string(Method->isConst() ? "const " : ""))
- .str();
+ MethodsString.emplace_back(generateOverrideString(Method, LangOpts));
}
- return MethodsString;
+
+ return llvm::join(MethodsString, "\n") + '\n';
}
Expected<Tweak::Effect> OverridePureVirtuals::apply(const Selection &Sel) {
// The correctness of this tweak heavily relies on the accurate population of
// these members.
- collectMissingPureVirtuals(Sel);
+ collectMissingPureVirtuals();
if (MissingMethodsByAccess.empty()) {
return llvm::make_error<llvm::StringError>(
@@ -214,30 +242,16 @@ Expected<Tweak::Effect> OverridePureVirtuals::apply(const Selection &Sel) {
// 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.
- // No methods to override for this access specifier.
- if (GroupIter == MissingMethodsByAccess.end() || GroupIter->second.empty())
- continue;
+
+ for (const auto &[AS, Methods] : MissingMethodsByAccess) {
+ assert(!Methods.empty());
std::string MethodsGroupString =
- generateOverridesStringForGroup(GroupIter->second, LangOpts);
+ generateOverridesStringForGroup(Methods, LangOpts);
auto *ExistingSpecLocIter = AccessSpecifierLocations.find(AS);
- if (ExistingSpecLocIter != AccessSpecifierLocations.end()) {
+ bool ASExists = ExistingSpecLocIter != AccessSpecifierLocations.end();
+ if (ASExists) {
// Access specifier section already exists in the class.
// Get location immediately *after* the colon.
SourceLocation InsertLoc =
@@ -246,7 +260,7 @@ Expected<Tweak::Effect> OverridePureVirtuals::apply(const Selection &Sel) {
// 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;
+ std::string InsertionText = MethodsGroupString;
tooling::Replacement Rep(SM, InsertLoc, 0, InsertionText);
if (auto Err = EditReplacements.add(Rep))
return llvm::Expected<Tweak::Effect>(std::move(Err));
@@ -254,12 +268,8 @@ Expected<Tweak::Effect> OverridePureVirtuals::apply(const Selection &Sel) {
// 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 +=
- getAccessSpelling(AS).str() + ":\n" + MethodsGroupString;
- IsFirstNewSection = false;
+ getAccessSpelling(AS).str() + ':' + MethodsGroupString;
}
}
@@ -269,10 +279,10 @@ Expected<Tweak::Effect> OverridePureVirtuals::apply(const Selection &Sel) {
// AppendLoc is the SourceLocation of the closing brace '}' of the class.
// The replacement will insert text *before* this closing brace.
SourceLocation AppendLoc = CurrentDeclDef->getBraceRange().getEnd();
- std::string FinalAppendText = NewSectionsToAppendText;
+ std::string FinalAppendText = std::move(NewSectionsToAppendText);
if (!CurrentDeclDef->decls_empty() || !EditReplacements.empty()) {
- FinalAppendText = "\n" + FinalAppendText;
+ FinalAppendText = '\n' + FinalAppendText;
}
// Create a replacement to append the new sections.
diff --git a/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp
index 83910221e1af3..b7dcbee1650ec 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp
@@ -44,7 +44,7 @@ class ^D : public B {
)cpp");
}
-TEST_F(OverridePureVirtualsTests, Availability) {
+TEST_F(OverridePureVirtualsTests, AvailabilityNoOverride) {
EXPECT_AVAILABLE(R"cpp(
class Base {
public:
@@ -58,7 +58,9 @@ class ^Derived : public Base {
};
)cpp");
+}
+TEST_F(OverridePureVirtualsTests, AvailabilityPartiallyOverridden) {
EXPECT_AVAILABLE(R"cpp(
class Base {
public:
@@ -96,11 +98,12 @@ virtual void F2(int P1, const int &P2) = 0;
class Derived : public Base {
public:
void F1() override {
- // TODO: Implement this pure virtual method
+ // 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
+ // TODO: Implement this pure virtual method.
static_assert(false, "Method `F2` is not implemented.");
}
};
@@ -136,7 +139,7 @@ virtual void F2() = 0;
class Derived : public Base {
public:
void F2() override {
- // TODO: Implement this pure virtual method
+ // TODO: Implement this pure virtual method.
static_assert(false, "Method `F2` is not implemented.");
}
@@ -172,13 +175,12 @@ class Base2 {
class Derived : public Base1, public Base2 {
public:
void func1() override {
- // TODO: Implement this pure virtual method
+ // 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
+ // TODO: Implement this pure virtual method.
static_assert(false, "Method `func2` is not implemented.");
}
};
@@ -208,7 +210,7 @@ class Base {
class Derived : public Base {
public:
void func(int, const S &, char *) override {
- // TODO: Implement this pure virtual method
+ // TODO: Implement this pure virtual method.
static_assert(false, "Method `func` is not implemented.");
}
};
@@ -239,7 +241,7 @@ class Right : virtual public Top {};
class Bottom : public Left, public Right {
public:
void diamond_func() override {
- // TODO: Implement this pure virtual method
+ // TODO: Implement this pure virtual method.
static_assert(false, "Method `diamond_func` is not implemented.");
}
};
@@ -277,11 +279,12 @@ class Derived : public Base {
int member; // Existing member
public:
void pub_func() override {
- // TODO: Implement this pure virtual method
+ // TODO: Implement this pure virtual method.
static_assert(false, "Method `pub_func` is not implemented.");
}
+
void pub_func2(char) const override {
- // TODO: Implement this pure virtual method
+ // TODO: Implement this pure virtual method.
static_assert(false, "Method `pub_func2` is not implemented.");
}
@@ -289,7 +292,7 @@ class Derived : public Base {
protected:
int prot_func(int x) const override {
- // TODO: Implement this pure virtual method
+ // TODO: Implement this pure virtual method.
static_assert(false, "Method `prot_func` is not implemented.");
}
};
@@ -329,18 +332,19 @@ class Derived : public Base {
int member; // Existing member
protected:
int prot_func(int x) const override {
- // TODO: Implement this pure virtual method
+ // TODO: Implement this pure virtual method.
static_assert(false, "Method `prot_func` is not implemented.");
}
void foo();
public:
void pub_func() override {
- // TODO: Implement this pure virtual method
+ // TODO: Implement this pure virtual method.
static_assert(false, "Method `pub_func` is not implemented.");
}
+
void pub_func2(char) const override {
- // TODO: Implement this pure virtual method
+ // TODO: Implement this pure virtual method.
static_assert(false, "Method `pub_func2` is not implemented.");
}
@@ -374,13 +378,12 @@ class Base {
class Derived : public Base {
public:
void foo() override {
- // TODO: Implement this pure virtual method
+ // TODO: Implement this pure virtual method.
static_assert(false, "Method `foo` is not implemented.");
}
-
protected:
void bar() override {
- // TODO: Implement this pure virtual method
+ // TODO: Implement this pure virtual method.
static_assert(false, "Method `bar` is not implemented.");
}
};
@@ -410,7 +413,7 @@ class Base {
class Derived : public Base {
public:
void func1() override {
- // TODO: Implement this pure virtual method
+ // TODO: Implement this pure virtual method.
static_assert(false, "Method `func1` is not implemented.");
}
@@ -420,6 +423,298 @@ class Derived : public Base {
EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
}
+TEST_F(OverridePureVirtualsTests, ConstexprSpecifier) {
+ ExtraArgs.push_back("-std=c++20");
+
+ constexpr auto Before = R"cpp(
+class B {
+public:
+ constexpr virtual int getValue() const = 0;
+};
+
+class ^D : public B {};
+)cpp";
+
+ constexpr auto Expected = R"cpp(
+class B {
+public:
+ constexpr virtual int getValue() const = 0;
+};
+
+class D : public B {
+public:
+ constexpr int getValue() const override {
+ // TODO: Implement this pure virtual method.
+ static_assert(false, "Method `getValue` is not implemented.");
+ }
+};
+)cpp";
+ auto Applied = apply(Before);
+ EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
+}
+
+TEST_F(OverridePureVirtualsTests, ConstevalSpecifier) {
+ ExtraArgs.push_back("-std=c++20");
+
+ constexpr auto Before = R"cpp(
+class B {
+public:
+ virtual consteval float calculate() = 0;
+};
+
+class ^D : public B {};
+)cpp";
+
+ constexpr auto Expected = R"cpp(
+class B {
+public:
+ virtual consteval float calculate() = 0;
+};
+
+class D : public B {
+public:
+ consteval float calculate() override {
+ // TODO: Implement this pure virtual method.
+ static_assert(false, "Method `calculate` is not implemented.");
+ }
+};
+)cpp";
+ auto Applied = apply(Before);
+ EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
+}
+
+TEST_F(OverridePureVirtualsTests, LValueRefQualifier) {
+ constexpr auto Before = R"cpp(
+class B {
+public:
+ virtual void process() & = 0;
+};
+
+class ^D : public B {};
+)cpp";
+
+ constexpr auto Expected = R"cpp(
+class B {
+public:
+ virtual void process() & = 0;
+};
+
+class D : public B {
+public:
+ void process() & override {
+ // TODO: Implement this pure virtual method.
+ static_assert(false, "Method `process` is not implemented.");
+ }
+};
+)cpp";
+ auto Applied = apply(Before);
+ EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
+}
+
+TEST_F(OverridePureVirtualsTests, RValueRefQualifier) {
+ constexpr auto Before = R"cpp(
+class B {
+public:
+ virtual bool isValid() && = 0;
+};
+
+class ^D : public B {};
+)cpp";
+
+ constexpr auto Expected = R"cpp(
+class B {
+public:
+ virtual bool isValid() && = 0;
+};
+
+class D : public B {
+public:
+ bool isValid() && override {
+ // TODO: Implement this pure virtual method.
+ static_assert(false, "Method `isValid` is not implemented.");
+ }
+};
+)cpp";
+ auto Applied = apply(Before);
+ EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
+}
+
+TEST_F(OverridePureVirtualsTests, SimpleTrailingReturnType) {
+ constexpr auto Before = R"cpp(
+class B {
+public:
+ virtual auto getStatus() -> bool = 0;
+};
+
+class ^D : public B {};
+)cpp";
+
+ constexpr auto Expected = R"cpp(
+class B {
+public:
+ virtual auto getStatus() -> bool = 0;
+};
+
+class D : public B {
+public:
+ auto getStatus() -> bool override {
+ // TODO: Implement this pure virtual method.
+ static_assert(false, "Method `getStatus` is not implemented.");
+ }
+};
+)cpp";
+ auto Applied = apply(Before);
+ EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
+}
+
+TEST_F(OverridePureVirtualsTests, ConstexprLValueRefAndTrailingReturn) {
+ ExtraArgs.push_back("-std=c++20");
+
+ constexpr auto Before = R"cpp(
+class B {
+public:
+ constexpr virtual auto getData() & -> const char * = 0;
+};
+
+class ^D : public B {};
+)cpp";
+
+ constexpr auto Expected = R"cpp(
+class B {
+public:
+ constexpr virtual auto getData() & -> const char * = 0;
+};
+
+class D : public B {
+public:
+ constexpr auto getData() & -> const char * override {
+ // TODO: Implement this pure virtual method.
+ static_assert(false, "Method `getData` is not implemented.");
+ }
+};
+)cpp";
+ auto Applied = apply(Before);
+ EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
+}
+
+TEST_F(OverridePureVirtualsTests, ConstevalRValueRefAndTrailingReturn) {
+ ExtraArgs.push_back("-std=c++20");
+
+ constexpr auto Before = R"cpp(
+class B {
+public:
+ virtual consteval auto foo() && -> double = 0;
+};
+
+class ^D : public B {};
+)cpp";
+
+ constexpr auto Expected = R"cpp(
+class B {
+public:
+ virtual consteval auto foo() && -> double = 0;
+};
+
+class D : public B {
+public:
+ consteval auto foo() && -> double override {
+ // TODO: Implement this pure virtual method.
+ static_assert(false, "Method `foo` is not implemented.");
+ }
+};
+)cpp";
+ auto Applied = apply(Before);
+ EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
+}
+
+TEST_F(OverridePureVirtualsTests, CombinedFeaturesWithTrailingReturnTypes) {
+ ExtraArgs.push_back("-std=c++20");
+
+ constexpr auto Before = R"cpp(
+class B {
+public:
+ virtual auto f1() & -> int = 0;
+ constexpr virtual auto f2() && -> int = 0;
+ virtual consteval auto f3() -> int = 0;
+ virtual auto f4() const & -> char = 0;
+ constexpr virtual auto f5() const && -> bool = 0;
+};
+
+class ^D : public B {};
+)cpp";
+
+ constexpr auto Expected = R"cpp(
+class B {
+public:
+ virtual auto f1() & -> int = 0;
+ constexpr virtual auto f2() && -> int = 0;
+ virtual consteval auto f3() -> int = 0;
+ virtual auto f4() const & -> char = 0;
+ constexpr virtual auto f5() const && -> bool = 0;
+};
+
+class D : public B {
+public:
+ auto f1() & -> int override {
+ // TODO: Implement this pure virtual method.
+ static_assert(false, "Method `f1` is not implemented.");
+ }
+
+ constexpr auto f2() && -> int override {
+ // TODO: Implement this pure virtual method.
+ static_assert(false, "Method `f2` is not implemented.");
+ }
+
+ consteval auto f3() -> int override {
+ // TODO: Implement this pure virtual method.
+ static_assert(false, "Method `f3` is not implemented.");
+ }
+
+ auto f4() const & -> char override {
+ // TODO: Implement this pure virtual method.
+ static_assert(false, "Method `f4` is not implemented.");
+ }
+
+ constexpr auto f5() const && -> bool override {
+ // TODO: Implement this pure virtual method.
+ static_assert(false, "Method `f5` is not implemented.");
+ }
+};
+)cpp";
+ auto Applied = apply(Before);
+ EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
+}
+
+TEST_F(OverridePureVirtualsTests, DefaultParameters) {
+ ExtraArgs.push_back("-std=c++20");
+
+ constexpr auto Before = R"cpp(
+class B {
+public:
+ virtual void foo(int var = 0) = 0;
+};
+
+class ^D : public B {};
+)cpp";
+
+ constexpr auto Expected = R"cpp(
+class B {
+public:
+ virtual void foo(int var = 0) = 0;
+};
+
+class D : public B {
+public:
+ void foo(int var = 0) override {
+ // TODO: Implement this pure virtual method.
+ static_assert(false, "Method `foo` is not implemented.");
+ }
+};
+)cpp";
+ auto Applied = apply(Before);
+ EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied;
+}
+
} // namespace
} // namespace clangd
} // namespace clang
>From 3b5391d392f028e44989423ba2363906e9a676c2 Mon Sep 17 00:00:00 2001
From: Marco Maia <marcogmaia at gmail.com>
Date: Wed, 14 May 2025 23:20:23 -0300
Subject: [PATCH 11/12] fixup! Apply @zwuis review
---
.../clangd/refactor/tweaks/OverridePureVirtuals.cpp | 1 -
1 file changed, 1 deletion(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
index 72343af5b8a29..830ac6f51aa34 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
@@ -7,7 +7,6 @@
//===----------------------------------------------------------------------===//
#include "refactor/Tweak.h"
-
#include "support/Token.h"
#include "clang/AST/ASTContext.h"
>From f9d8b8716207faa6c299c763581a0a6f5ef22180 Mon Sep 17 00:00:00 2001
From: Marco Maia <marcogmaia at gmail.com>
Date: Sun, 18 May 2025 08:29:23 -0300
Subject: [PATCH 12/12] Remove unused variable
Some minor improvements:
- Add assert instead of a check that is always false
- Add final on tweak class
---
.../refactor/tweaks/OverridePureVirtuals.cpp | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
index 830ac6f51aa34..cb0d5a910cba1 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp
@@ -58,7 +58,7 @@ std::string removePureVirtualSyntax(const std::string &MethodDecl,
return DeclString;
}
-class OverridePureVirtuals : public Tweak {
+class OverridePureVirtuals final : public Tweak {
public:
const char *id() const final; // defined by REGISTER_TWEAK.
bool prepare(const Selection &Sel) override;
@@ -77,7 +77,6 @@ class OverridePureVirtuals : public Tweak {
MissingMethodsByAccess;
// Stores the source locations of existing access specifiers in CurrentDecl.
llvm::MapVector<AccessSpecifier, SourceLocation> AccessSpecifierLocations;
-
// Helper function to gather information before applying the tweak.
void collectMissingPureVirtuals();
};
@@ -213,8 +212,8 @@ std::string generateOverridesStringForGroup(
const LangOptions &LangOpts) {
llvm::SmallVector<std::string> MethodsString;
MethodsString.reserve(Methods.size());
- std::string SS;
- for (const auto *Method : Methods) {
+
+ for (const CXXMethodDecl *Method : Methods) {
MethodsString.emplace_back(generateOverrideString(Method, LangOpts));
}
@@ -225,11 +224,9 @@ Expected<Tweak::Effect> OverridePureVirtuals::apply(const Selection &Sel) {
// The correctness of this tweak heavily relies on the accurate population of
// these members.
collectMissingPureVirtuals();
-
- if (MissingMethodsByAccess.empty()) {
- return llvm::make_error<llvm::StringError>(
- "No pure virtual methods to override.", llvm::inconvertibleErrorCode());
- }
+ // The `prepare` should prevent this. If the prepare identifies an abstract
+ // method, then is must have missing methods.
+ assert(!MissingMethodsByAccess.empty());
const auto &SM = Sel.AST->getSourceManager();
const auto &LangOpts = Sel.AST->getLangOpts();
More information about the cfe-commits
mailing list