llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: Frank Winklmeier (fwinkl) <details> <summary>Changes</summary> Add a new `AllowVirtual` option to 'modernize-use-override', which does not remove the `virtual` specifier. This is useful because some coding guidelines may suggest to use `virtual ... override` for clarity. See the new unit test for examples. --- Full diff: https://github.com/llvm/llvm-project/pull/144916.diff 4 Files Affected: - (modified) clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp (+10-3) - (modified) clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h (+1) - (modified) clang-tools-extra/docs/clang-tidy/checks/modernize/use-override.rst (+7-1) - (added) clang-tools-extra/test/clang-tidy/checkers/modernize/use-override-allow-virtual.cpp (+25) ``````````diff diff --git a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp index fd5bd9f0b181b..c18bd68a99b37 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp @@ -22,6 +22,7 @@ UseOverrideCheck::UseOverrideCheck(StringRef Name, ClangTidyContext *Context) IgnoreTemplateInstantiations( Options.get("IgnoreTemplateInstantiations", false)), AllowOverrideAndFinal(Options.get("AllowOverrideAndFinal", false)), + AllowVirtual(Options.get("AllowVirtual", false)), OverrideSpelling(Options.get("OverrideSpelling", "override")), FinalSpelling(Options.get("FinalSpelling", "final")) {} @@ -30,6 +31,7 @@ void UseOverrideCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "IgnoreTemplateInstantiations", IgnoreTemplateInstantiations); Options.store(Opts, "AllowOverrideAndFinal", AllowOverrideAndFinal); + Options.store(Opts, "AllowVirtual", AllowVirtual); Options.store(Opts, "OverrideSpelling", OverrideSpelling); Options.store(Opts, "FinalSpelling", FinalSpelling); } @@ -111,15 +113,19 @@ void UseOverrideCheck::check(const MatchFinder::MatchResult &Result) { unsigned KeywordCount = HasVirtual + HasOverride + HasFinal; if ((!OnlyVirtualSpecified && KeywordCount == 1) || + (HasVirtual && HasOverride && AllowVirtual) || (!HasVirtual && HasOverride && HasFinal && AllowOverrideAndFinal)) return; // Nothing to do. std::string Message; if (OnlyVirtualSpecified) { - Message = "prefer using '%0' or (rarely) '%1' instead of 'virtual'"; + if (AllowVirtual) + Message = "add '%0' or (rarely) '%1' for 'virtual' function"; + else + Message = "prefer using '%0' or (rarely) '%1' instead of 'virtual'"; } else if (KeywordCount == 0) { Message = "annotate this function with '%0' or (rarely) '%1'"; - } else { + } else if (!AllowVirtual) { StringRef Redundant = HasVirtual ? (HasOverride && HasFinal && !AllowOverrideAndFinal ? "'virtual' and '%0' are" @@ -226,7 +232,8 @@ void UseOverrideCheck::check(const MatchFinder::MatchResult &Result) { CharSourceRange::getTokenRange(OverrideLoc, OverrideLoc)); } - if (HasVirtual) { + // Remove virtual unless requested otherwise + if (HasVirtual && !AllowVirtual) { for (Token Tok : Tokens) { if (Tok.is(tok::kw_virtual)) { std::optional<Token> NextToken = diff --git a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h index 2c624f48fcc85..6879221ab4284 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h +++ b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h @@ -29,6 +29,7 @@ class UseOverrideCheck : public ClangTidyCheck { const bool IgnoreDestructors; const bool IgnoreTemplateInstantiations; const bool AllowOverrideAndFinal; + const bool AllowVirtual; const StringRef OverrideSpelling; const StringRef FinalSpelling; }; diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-override.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-override.rst index f8f34794af749..d3ed73ef35a51 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-override.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-override.rst @@ -4,7 +4,8 @@ modernize-use-override ====================== Adds ``override`` (introduced in C++11) to overridden virtual functions and -removes ``virtual`` from those functions as it is not required. +removes ``virtual`` from those functions (unless the `AllowVirtual` option is +used). ``virtual`` on non base class implementations was used to help indicate to the user that a function was virtual. C++ compilers did not use the presence of @@ -40,6 +41,11 @@ Options members, such as ``gcc -Wsuggest-override``/``gcc -Werror=suggest-override``. Default is `false`. +.. option:: AllowVirtual + + If set to `true`, ``virtual`` will not be removed by this check when adding + ``override`` or ``final``. Default is `false`. + .. option:: OverrideSpelling Specifies a macro to use instead of ``override``. This is useful when diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-override-allow-virtual.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-override-allow-virtual.cpp new file mode 100644 index 0000000000000..17be170e0d9cd --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-override-allow-virtual.cpp @@ -0,0 +1,25 @@ +// RUN: %check_clang_tidy %s modernize-use-override %t -- \ +// RUN: -config="{CheckOptions: {modernize-use-override.AllowVirtual: true}}" + +struct Base { + virtual ~Base(); + virtual void a(); + virtual void b(); + virtual void c(); +}; + +struct Derived : public Base { + virtual ~Derived() override; + + virtual void a() override; + // CHECK-MESSAGES-NOT: warning: + // CHECK-FIXES: {{^}} virtual void a() override; + + virtual void b(); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: add 'override' + // CHECK-FIXES: {{^}} virtual void b() override; + + void c(); + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: annotate + // CHECK-FIXES: {{^}} void c() override; +}; `````````` </details> https://github.com/llvm/llvm-project/pull/144916 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
