https://github.com/marcogmaia created https://github.com/llvm/llvm-project/pull/184023
closes https://github.com/clangd/clangd/issues/2611 - Preserve attributes format when generating override stubs - Preserve comments and whitespace in method declarations - Add tests for attributes, comments, and macro-generated methods >From d54367c5c00720b2000d8537c414095f37ce3aae Mon Sep 17 00:00:00 2001 From: Marco Maia <[email protected]> Date: Sun, 1 Mar 2026 15:03:20 -0300 Subject: [PATCH] [clangd] Fix OverridePureVirtuals to preserve attributes and formatting closes https://github.com/clangd/clangd/issues/2611 - Preserve attributes format when generating override stubs - Preserve comments and whitespace in method declarations - Add tests for attributes, comments, and macro-generated methods --- .../refactor/tweaks/OverridePureVirtuals.cpp | 87 +++++++--- .../tweaks/OverridePureVirtualsTests.cpp | 153 +++++++++++++++++- 2 files changed, 217 insertions(+), 23 deletions(-) diff --git a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp index b557066d979f5..43b15bbcec94a 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp @@ -92,7 +92,8 @@ namespace clang { namespace clangd { namespace { -// This function removes the "virtual" and the "= 0" at the end; +// This function removes the "virtual" and the "= 0" at the end while +// preserving original formatting and attributes. // e.g.: // "virtual void foo(int var = 0) = 0" // input. // "void foo(int var = 0)" // output. @@ -101,30 +102,63 @@ std::string removePureVirtualSyntax(const std::string &MethodDecl, assert(!MethodDecl.empty()); TokenStream TS = lex(MethodDecl, LangOpts); + auto Tokens = TS.tokens(); - std::string DeclString; - for (const clangd::Token &Tk : TS.tokens()) { - if (Tk.Kind == clang::tok::raw_identifier && Tk.text() == "virtual") + // Find the pure-specifier (= 0) by searching backwards. + size_t PureEqualIndex = Tokens.size(); + for (int J = static_cast<int>(Tokens.size()) - 1; J >= 1; --J) { + auto Kind = Tokens[J].Kind; + if (Kind == tok::eof || Kind == tok::semi || Kind == tok::comment) 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") + if (Tokens[J].text() == "0" && Tokens[J - 1].Kind == tok::equal) + PureEqualIndex = J - 1; + + break; + } + + std::string Result; + const char *Pos = MethodDecl.data(); + const char *End = MethodDecl.data() + MethodDecl.size(); + + // Build the cleaned declaration by iterating through tokens: + // 1. Skip 'virtual' keyword; + // 2. Skip pure-specifier (= 0) and trailing semicolon; + // 3. Preserve all other tokens and their trivia (whitespace/comments). + for (size_t I = 0; I < Tokens.size(); ++I) { + const auto &Tk = Tokens[I]; + if (Tk.Kind == tok::eof) break; - DeclString += Tk.text(); - if (Tk.Kind != tok::l_paren && Next.Kind != tok::comma && - Next.Kind != tok::r_paren && Next.Kind != tok::l_paren && - Tk.Kind != tok::coloncolon && Next.Kind != tok::coloncolon) - DeclString += ' '; + // Skip 'virtual' keyword. + if (Tk.Kind == tok::raw_identifier && Tk.text() == "virtual") { + if (Tk.text().begin() > Pos) + Result.append(Pos, Tk.text().begin() - Pos); + Pos = Tk.text().end(); + while (Pos < End && (*Pos == ' ' || *Pos == '\t')) + ++Pos; + continue; + } + + // Skip pure-specifier (= 0). + if (I == PureEqualIndex) { + if (Tk.text().begin() > Pos) + Result.append(Pos, Tk.text().begin() - Pos); + Pos = Tokens[I + 1].text().end(); + if (I + 2 < Tokens.size() && Tokens[I + 1].Kind == tok::semi) + Pos = Tokens[I + 2].text().end(); + continue; + } + + // Keep token and its trivia. + if (Tk.text().end() > Pos) { + Result.append(Pos, Tk.text().end() - Pos); + Pos = Tk.text().end(); + } } - // Trim the last whitespace. - if (DeclString.back() == ' ') - DeclString.pop_back(); - return DeclString; + // trim() handles any trailing spaces left behind by skipping "= 0". + return llvm::StringRef(Result).trim().str(); } class OverridePureVirtuals final : public Tweak { @@ -266,9 +300,20 @@ void OverridePureVirtuals::collectMissingPureVirtuals() { std::string generateOverrideString(const CXXMethodDecl *Method, const LangOptions &LangOpts) { - std::string MethodDecl; - auto OS = llvm::raw_string_ostream(MethodDecl); - Method->print(OS); + // Try to get the original source text first to preserve original formatting, + // comments, and attributes (e.g., [[nodiscard]]). This is important because + // printing the declaration via AST can lose formatting and some attributes. + const SourceManager &SM = Method->getASTContext().getSourceManager(); + auto Range = CharSourceRange::getTokenRange(Method->getSourceRange()); + std::string MethodDecl = Lexer::getSourceText(Range, SM, LangOpts).str(); + + // Fallback to printing if source text is unavailable (e.g., for + // macro-generated methods that have no source location). + if (MethodDecl.empty()) { + PrintingPolicy Policy(LangOpts); + auto OS = llvm::raw_string_ostream(MethodDecl); + Method->print(OS, Policy); + } return llvm::formatv( "\n {0} override {{\n" diff --git a/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp index 72095ab2f5982..c724cfeb2d4f9 100644 --- a/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp @@ -102,7 +102,7 @@ class Derived : public Base { static_assert(false, "Method `F1` is not implemented."); } - void F2(int P1, const int & P2) override { + void F2(int P1, const int &P2) override { // TODO: Implement this pure virtual method. static_assert(false, "Method `F2` is not implemented."); } @@ -209,7 +209,7 @@ class Base { class Derived : public Base { public: - void func(int, const S &, char *) override { + void func(int, const S&, char*) override { // TODO: Implement this pure virtual method. static_assert(false, "Method `func` is not implemented."); } @@ -754,6 +754,155 @@ class D : public B { EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied; } +TEST_F(OverridePureVirtualsTests, NoDiscardAttribute) { + constexpr auto Before = R"cpp( +class Base { +public: + <:<:nodiscard:>:> virtual int foo() const = 0; +}; + +class ^D : Base {}; +)cpp"; + + constexpr auto Expected = R"cpp( +class Base { +public: + <:<:nodiscard:>:> virtual int foo() const = 0; +}; + +class D : Base { +public: + <:<:nodiscard:>:> int foo() const 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, NoDiscardAttributeWithReason) { + ExtraArgs.push_back("-std=c++20"); + + constexpr auto Before = R"cpp( +class Base { +public: + <:<:nodiscard("reason 42"):>:> virtual int foo() const = 0; +}; + +class ^D : Base {}; +)cpp"; + + constexpr auto Expected = R"cpp( +class Base { +public: + <:<:nodiscard("reason 42"):>:> virtual int foo() const = 0; +}; + +class D : Base { +public: + <:<:nodiscard("reason 42"):>:> int foo() const 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, CommentsInDeclaration) { + constexpr auto Before = R"cpp( +class Base { +public: + virtual void foo(int /* size */ x) = 0; +}; + +class ^D : Base {}; +)cpp"; + + constexpr auto Expected = R"cpp( +class Base { +public: + virtual void foo(int /* size */ x) = 0; +}; + +class D : Base { +public: + void foo(int /* size */ x) 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, MacroGeneratedMethod) { + constexpr auto Before = R"cpp( +#define DECLARE_PURE_VIRTUAL(ReturnType, MethodName) \ + virtual ReturnType MethodName() = 0; + +class Base { +public: + DECLARE_PURE_VIRTUAL(int, foo) +}; + +class ^D : public Base {}; +)cpp"; + + constexpr auto Expected = R"cpp( +#define DECLARE_PURE_VIRTUAL(ReturnType, MethodName) \ + virtual ReturnType MethodName() = 0; + +class Base { +public: + DECLARE_PURE_VIRTUAL(int, foo) +}; + +class D : public Base { +public: + int foo() 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, SkipCommentsAfterPureSpecifier) { + constexpr auto Before = R"cpp( +class Base { +public: + virtual int foo() const = 0 /* hi */; // random comment. +}; + +class ^D : Base {}; +)cpp"; + + constexpr auto Expected = R"cpp( +class Base { +public: + virtual int foo() const = 0 /* hi */; // random comment. +}; + +class D : Base { +public: + int foo() const 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 [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
