llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tidy
@llvm/pr-subscribers-clang-tools-extra
Author: Victor Chernyakin (localspook)
<details>
<summary>Changes</summary>
This removes some of the incredibly painful manual lexing in this check, yay!
Fixes #<!-- -->138486. Also fixes the following case with `= delete("...")` in
C++26: https://godbolt.org/z/dddfY15Wq.
---
Full diff: https://github.com/llvm/llvm-project/pull/172196.diff
4 Files Affected:
- (modified) clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
(+9-71)
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5)
- (added)
clang-tools-extra/test/clang-tidy/checkers/modernize/use-override-cxx26.cpp
(+11)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/modernize/use-override.cpp (+9-3)
``````````diff
diff --git a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
index e2e7bba46f2c4..3bcfd9ad6bc27 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
@@ -141,90 +141,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.
``````````
</details>
https://github.com/llvm/llvm-project/pull/172196
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
