poelmanc created this revision.
poelmanc added reviewers: alexfh, djasper.
Herald added subscribers: cfe-commits, mgehre.
Herald added a project: clang.
In addition to adding `override` wherever possible, clang-tidy's
`modernize-use-override` nicely removes `virtual` when `override` or `final` is
specified, and further removes `override` when `final` is specified. While this
is great default behavior, when code needs to be compiled with `gcc` at high
warning levels that include `gcc -Wsuggest-override` or `gcc
-Werror=suggest-override`, clang-tidy's removal of the redundant `override`
keyword causes gcc to emit a warning or error. This discrepancy / conflict has
been noted by others including a comment on Stack Overflow
<https://stackoverflow.com/questions/29145476/requiring-virtual-function-overrides-to-use-override-keyword>
and by Mozzilla's Firefox developers
<https://bugzilla.mozilla.org/show_bug.cgi?id=1430669>.
This patch adds an AllowOverrideAndFinal option defaulting to `0` - thus
preserving current behavior - that when enabled allows both `override` and
`final` to co-exist, while still fixing all other issues.
The patch includes a test file verifying all combinations of
``virtual``/``override``/``final``, and mentions the new option in the release
notes.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D70165
Files:
clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/modernize-use-override.rst
clang-tools-extra/test/clang-tidy/checkers/modernize-use-override-allow-override-and-final.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-override-allow-override-and-final.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-override-allow-override-and-final.cpp
@@ -0,0 +1,40 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN: -config="{CheckOptions: [{key: modernize-use-override.AllowOverrideAndFinal, value: 1}]}"
+
+struct Base {
+ virtual ~Base();
+ virtual void a();
+ virtual void b();
+ virtual void c();
+ virtual void d();
+ virtual void e();
+ virtual void f();
+ virtual void g();
+ virtual void h();
+ virtual void i();
+};
+
+struct Simple : public Base {
+ virtual ~Simple();
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override]
+ // CHECK-FIXES: {{^}} ~Simple() override;
+ virtual void a() override;
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'override' [modernize-use-override]
+ // CHECK-FIXES: {{^}} void a() override;
+ virtual void b() final;
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'final' [modernize-use-override]
+ // CHECK-FIXES: {{^}} void b() final;
+ virtual void c() final override;
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'final' [modernize-use-override]
+ // CHECK-FIXES: {{^}} void c() final override;
+ virtual void d() override final;
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'final' [modernize-use-override]
+ // CHECK-FIXES: {{^}} void d() override final;
+ void e() final override;
+ void f() override final;
+ void g() final;
+ void h() override;
+ void i();
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: annotate this function with 'override' or (rarely) 'final' [modernize-use-override]
+ // CHECK-FIXES: {{^}} void i() override;
+};
Index: clang-tools-extra/docs/clang-tidy/checks/modernize-use-override.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/modernize-use-override.rst
+++ clang-tools-extra/docs/clang-tidy/checks/modernize-use-override.rst
@@ -27,6 +27,14 @@
If set to non-zero, this check will not diagnose destructors. Default is `0`.
+.. option:: AllowOverrideAndFinal
+
+ If set to non-zero, this check will not diagnose ``override`` as redundant
+ with ``final``. This is useful when code will be compiled by a compiler with
+ warning/error checking flags requiring ``override`` explicitly on overriden
+ members, such as ``gcc -Wsuggest-override``/``gcc -Werror=suggest-override``.
+ Default is `0`.
+
.. option:: OverrideSpelling
Specifies a macro to use instead of ``override``. This is useful when
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -153,6 +153,12 @@
Finds non-static member functions that can be made ``const``
because the functions don't use ``this`` in a non-const way.
+- Improved :doc:`modernize-use-override
+ <clang-tidy/checks/modernize-use-override>` check.
+
+ The check now supports the ``AllowOverrideAndFinal`` option to eliminate
+ conflicts with ``gcc -Wsuggest-override`` or ``gcc -Werror=suggest-override``.
+
Improvements to include-fixer
-----------------------------
Index: clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h
+++ clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h
@@ -26,6 +26,7 @@
private:
const bool IgnoreDestructors;
+ const bool AllowOverrideAndFinal;
const std::string OverrideSpelling;
const std::string FinalSpelling;
};
Index: clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
@@ -20,11 +20,13 @@
UseOverrideCheck::UseOverrideCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
IgnoreDestructors(Options.get("IgnoreDestructors", false)),
+ AllowOverrideAndFinal(Options.get("AllowOverrideAndFinal", false)),
OverrideSpelling(Options.get("OverrideSpelling", "override")),
FinalSpelling(Options.get("FinalSpelling", "final")) {}
void UseOverrideCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "IgnoreDestructors", IgnoreDestructors);
+ Options.store(Opts, "AllowOverrideAndFinal", AllowOverrideAndFinal);
Options.store(Opts, "OverrideSpelling", OverrideSpelling);
Options.store(Opts, "FinalSpelling", FinalSpelling);
}
@@ -103,7 +105,8 @@
bool OnlyVirtualSpecified = HasVirtual && !HasOverride && !HasFinal;
unsigned KeywordCount = HasVirtual + HasOverride + HasFinal;
- if (!OnlyVirtualSpecified && KeywordCount == 1)
+ if ((!OnlyVirtualSpecified && KeywordCount == 1) ||
+ (!HasVirtual && HasOverride && HasFinal && AllowOverrideAndFinal))
return; // Nothing to do.
std::string Message;
@@ -113,8 +116,9 @@
Message = "annotate this function with '%0' or (rarely) '%1'";
} else {
StringRef Redundant =
- HasVirtual ? (HasOverride && HasFinal ? "'virtual' and '%0' are"
- : "'virtual' is")
+ HasVirtual ? (HasOverride && HasFinal && !AllowOverrideAndFinal
+ ? "'virtual' and '%0' are"
+ : "'virtual' is")
: "'%0' is";
StringRef Correct = HasFinal ? "'%1'" : "'%0'";
@@ -211,7 +215,7 @@
Diag << FixItHint::CreateInsertion(InsertLoc, ReplacementText);
}
- if (HasFinal && HasOverride) {
+ if (HasFinal && HasOverride && !AllowOverrideAndFinal) {
SourceLocation OverrideLoc = Method->getAttr<OverrideAttr>()->getLocation();
Diag << FixItHint::CreateRemoval(
CharSourceRange::getTokenRange(OverrideLoc, OverrideLoc));
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
