Skip to content

[clang-tidy] Add option to keep virtual in 'modernize-use-override' #144916

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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")) {}

Expand All @@ -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);
}
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -226,7 +232,8 @@ void UseOverrideCheck::check(const MatchFinder::MatchResult &Result) {
CharSourceRange::getTokenRange(OverrideLoc, OverrideLoc));
}

if (HasVirtual) {
// Remove virtual unless requested otherwise
Copy link
Author

@fwinkl fwinkl Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that even when AllowVirtual is used, the check does not add any missing virtual keywords. If this is preferred one could instead add a UseVirtualAndOverride option which would then also add a missing virtual specifier.

But as this is a bit more work I would first like to judge the interest in that feature in general.

if (HasVirtual && !AllowVirtual) {
for (Token Tok : Tokens) {
if (Tok.is(tok::kw_virtual)) {
std::optional<Token> NextToken =
Expand Down
1 change: 1 addition & 0 deletions clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Comment on lines +15 to +16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should just write nothing. By default, tests will fail if there will be a message


virtual void b();
// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: add 'override'
// CHECK-FIXES: {{^}} virtual void b() override;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use {{^}}, just match string as a whole


void c();
// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: annotate
// CHECK-FIXES: {{^}} void c() override;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto {{^}}

};
Loading