https://github.com/localspook updated https://github.com/llvm/llvm-project/pull/172196
>From b284d180abdb752bd1a4fc2a1160ee1e9a73ea5c Mon Sep 17 00:00:00 2001 From: Victor Chernyakin <[email protected]> Date: Sat, 13 Dec 2025 21:46:57 -0800 Subject: [PATCH] [clang-tidy] Fix misplaced fix-its in `modernize-use-override` --- .../clang-tidy/modernize/UseOverrideCheck.cpp | 84 ++----------------- clang-tools-extra/docs/ReleaseNotes.rst | 5 ++ .../checkers/modernize/use-override-cxx26.cpp | 11 +++ .../checkers/modernize/use-override.cpp | 12 ++- 4 files changed, 34 insertions(+), 78 deletions(-) create mode 100644 clang-tools-extra/test/clang-tidy/checkers/modernize/use-override-cxx26.cpp diff --git a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp index e2e7bba46f2c4..dd516f8e51264 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp @@ -84,10 +84,6 @@ parseTokens(CharSourceRange Range, const MatchFinder::MatchResult &Result) { return Tokens; } -static StringRef getText(const Token &Tok, const SourceManager &Sources) { - return {Sources.getCharacterData(Tok.getLocation()), Tok.getLength()}; -} - void UseOverrideCheck::check(const MatchFinder::MatchResult &Result) { const auto *Method = Result.Nodes.getNodeAs<FunctionDecl>("method"); const SourceManager &Sources = *Result.SourceManager; @@ -141,90 +137,28 @@ void UseOverrideCheck::check(const MatchFinder::MatchResult &Result) { if (!FileRange.isValid()) return; - // FIXME: Instead of re-lexing and looking for specific macros such as - // 'ABSTRACT', properly store the location of 'virtual' and '= 0' in each - // FunctionDecl. + // FIXME: Instead of re-lexing and looking for the 'virtual' token, + // store the location of 'virtual' in each FunctionDecl. SmallVector<Token, 16> Tokens = parseTokens(FileRange, Result); // Add 'override' on inline declarations that don't already have it. if (!HasFinal && !HasOverride) { - SourceLocation InsertLoc; - std::string ReplacementText = (OverrideSpelling + " ").str(); - const SourceLocation MethodLoc = Method->getLocation(); - - for (const Token T : Tokens) { - if (T.is(tok::kw___attribute) && - !Sources.isBeforeInTranslationUnit(T.getLocation(), MethodLoc)) { - InsertLoc = T.getLocation(); - break; - } - } - - if (Method->hasAttrs()) { - for (const clang::Attr *A : Method->getAttrs()) { - if (!A->isImplicit() && !A->isInherited()) { - const SourceLocation Loc = - Sources.getExpansionLoc(A->getRange().getBegin()); - if ((!InsertLoc.isValid() || - Sources.isBeforeInTranslationUnit(Loc, InsertLoc)) && - !Sources.isBeforeInTranslationUnit(Loc, MethodLoc)) - InsertLoc = Loc; - } - } - } - - if (InsertLoc.isInvalid() && Method->doesThisDeclarationHaveABody() && - Method->getBody() && !Method->isDefaulted()) { - // For methods with inline definition, add the override keyword at the - // end of the declaration of the function, but prefer to put it on the - // same line as the declaration if the beginning brace for the start of - // the body falls on the next line. - ReplacementText = (" " + OverrideSpelling).str(); - auto *LastTokenIter = std::prev(Tokens.end()); - // When try statement is used instead of compound statement as - // method body - insert override keyword before it. - if (LastTokenIter->is(tok::kw_try)) - LastTokenIter = std::prev(LastTokenIter); - InsertLoc = LastTokenIter->getEndLoc(); - } - - if (!InsertLoc.isValid()) { - // For declarations marked with "= 0" or "= [default|delete]", the end - // location will point until after those markings. Therefore, the override - // keyword shouldn't be inserted at the end, but before the '='. - if (Tokens.size() > 2 && - (getText(Tokens.back(), Sources) == "0" || - Tokens.back().is(tok::kw_default) || - Tokens.back().is(tok::kw_delete)) && - getText(Tokens[Tokens.size() - 2], Sources) == "=") { - InsertLoc = Tokens[Tokens.size() - 2].getLocation(); - // Check if we need to insert a space. - if ((Tokens[Tokens.size() - 2].getFlags() & Token::LeadingSpace) == 0) - ReplacementText = (" " + OverrideSpelling + " ").str(); - } else if (getText(Tokens.back(), Sources) == "ABSTRACT") - InsertLoc = Tokens.back().getLocation(); - } - - if (!InsertLoc.isValid()) { - InsertLoc = FileRange.getEnd(); - ReplacementText = (" " + OverrideSpelling).str(); - } - // If the override macro has been specified just ensure it exists, // if not don't apply a fixit but keep the warning. if (OverrideSpelling != "override" && !Context.Idents.get(OverrideSpelling).hasMacroDefinition()) return; - Diag << FixItHint::CreateInsertion(InsertLoc, ReplacementText); + Diag << FixItHint::CreateInsertion( + Lexer::getLocForEndOfToken( + Method->getTypeSourceInfo()->getTypeLoc().getEndLoc(), 0, Sources, + getLangOpts()), + (" " + OverrideSpelling).str()); } - if (HasFinal && HasOverride && !AllowOverrideAndFinal) { - const SourceLocation OverrideLoc = - Method->getAttr<OverrideAttr>()->getLocation(); + if (HasFinal && HasOverride && !AllowOverrideAndFinal) Diag << FixItHint::CreateRemoval( - CharSourceRange::getTokenRange(OverrideLoc, OverrideLoc)); - } + Method->getAttr<OverrideAttr>()->getLocation()); if (HasVirtual) { for (const Token Tok : Tokens) { diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index d1fb1cba3e45a..8979e393f8363 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -508,6 +508,11 @@ Changes in existing checks on Windows when the check was enabled with a 32-bit :program:`clang-tidy` binary. +- Improved :doc:`modernize-use-override + <clang-tidy/checks/modernize/use-override>` by fixing an issue where + the check would sometimes suggest inserting ``override`` in an invalid + place. + - Improved :doc:`modernize-use-scoped-lock <clang-tidy/checks/modernize/use-scoped-lock>` check by fixing a crash on malformed code (common when using :program:`clang-tidy` through diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-override-cxx26.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-override-cxx26.cpp new file mode 100644 index 0000000000000..e0ced1def9b6a --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-override-cxx26.cpp @@ -0,0 +1,11 @@ +// RUN: %check_clang_tidy -std=c++26-or-later %s modernize-use-override,cppcoreguidelines-explicit-virtual-functions %t + +struct Base { + virtual void f() = delete(""); +}; + +struct Derived : Base { + virtual void f() = delete(""); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' + // CHECK-FIXES: void f() override = delete(""); +}; diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-override.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-override.cpp index 8e19fdd4a0a92..c33b8ae2b24fc 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-override.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-override.cpp @@ -48,6 +48,8 @@ struct Base { virtual void t() throw(); virtual void il(IntPair); + + virtual void u(int x __attribute__((unused))) {} }; struct SimpleCases : public Base { @@ -82,11 +84,11 @@ struct SimpleCases : public Base { virtual void f()=0; // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using - // CHECK-FIXES: void f() override =0; + // CHECK-FIXES: void f() override=0; virtual void f2() const=0; // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using - // CHECK-FIXES: void f2() const override =0; + // CHECK-FIXES: void f2() const override=0; virtual void g() ABSTRACT; // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using @@ -132,6 +134,10 @@ struct SimpleCases : public Base { virtual /* */ void g2(); // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' // CHECK-FIXES: /* */ void g2() override; + + virtual void u(int x __attribute__((unused))); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' + // CHECK-FIXES: void u(int x __attribute__((unused))) override; }; // CHECK-MESSAGES-NOT: warning: @@ -308,7 +314,7 @@ struct MembersOfSpecializations : public Base2 { // CHECK-FIXES: void a() override; }; template <> void MembersOfSpecializations<3>::a() {} -void ff() { MembersOfSpecializations<3>().a(); }; +void ff() { MembersOfSpecializations<3>().a(); } // In case try statement is used as a method body, // make sure that override fix is placed before try keyword. _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
