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.m...@iarasystems.com.br> Date: Sat, 10 May 2025 00:48:39 -0300 Subject: [PATCH 01/10] [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 <marcogm...@gmail.com> Date: Sat, 10 May 2025 08:57:45 -0300 Subject: [PATCH 02/10] 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 <marcogm...@gmail.com> Date: Sat, 10 May 2025 18:44:18 -0300 Subject: [PATCH 03/10] 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 <marcogm...@gmail.com> Date: Sat, 10 May 2025 20:22:56 -0300 Subject: [PATCH 04/10] 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 <marcogm...@gmail.com> Date: Sat, 10 May 2025 20:37:01 -0300 Subject: [PATCH 05/10] 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 <marcogm...@gmail.com> Date: Sat, 10 May 2025 21:34:57 -0300 Subject: [PATCH 06/10] 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 <marcogm...@gmail.com> Date: Sat, 10 May 2025 21:39:22 -0300 Subject: [PATCH 07/10] 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 <marcogm...@gmail.com> Date: Sun, 11 May 2025 03:33:22 -0300 Subject: [PATCH 08/10] 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 <marcogm...@gmail.com> Date: Sun, 11 May 2025 20:01:34 -0300 Subject: [PATCH 09/10] 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 <marcogm...@gmail.com> Date: Wed, 14 May 2025 23:03:37 -0300 Subject: [PATCH 10/10] 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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
