[clang-tidy] added cppcoreguidelines-explicit-virtual-functions

[lewmpk]

Summary

Addresses the bugzilla bug #30397. (https://bugs.llvm.org/show_bug.cgi?id=30397)
modernize-use-override suggests that destructors require the override specifier and the CPP core guidelines do not recommend this.

[JonasToth]

Welcome to the LLVM community and thank you for the patch lewmpk!

Please add unit tests for the changes you made to the check. They live in clang-tools-extra/test/clang-tidy/modernize-.....
It is common to add a additional test-file to ensure the configuration options do work as expected.
You can get inspiration from other checks that provide configuration capability (e.g. https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-bool-literals.html#options)
for reference.

If you have any questions don't hesitate to ask!

P.S: Usually we upload the patches with full context (https://llvm.org/docs/DeveloperPolicy.html#making-and-submitting-a-patch) for the reviewer to see the change in context.

clang-tidy/modernize/UseOverrideCheck.cpp

23 ↗

(On Diff #188588)

Please ellide the braces for the single-statements (this if and the else branch)

clang-tidy/modernize/UseOverrideCheck.h

23 ↗

(On Diff #188588)

That requires additional methods for the function for the configuration to function properly. Please see other checks and implement that the same way.

27 ↗

(On Diff #188588)

please use private instead

[JonasToth]

I think this change is worth mentioning in the release notes as well (cte/docs/ReleaseNotes.rst)

[MyDeveloperDay]

Please resubmit this patch with full context

git diff --cached -U999999 > patch_to_submitt.diff

You need to add some documention describing the new option into the check in

docs/clang-tidy/checks/modernize.... .rst

[lewmpk]

• addressed comments
• provided tests
• updated documentation
• updated release notes

[lewmpk]

clang-tidy/modernize/UseOverrideCheck.h

23 ↗

(On Diff #188588)

Could you elaborate? I looked at other checks and they seem to implement it the same way.

/llvm/tools/clang/tools/extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp:74

CheckFunctionCalls(Options.get("CheckFunctionCalls", false)),

[alexfh]

Thank you for the contribution! Please see the comments inline.

clang-tidy/modernize/UseOverrideCheck.cpp

22 ↗

(On Diff #188622)

Please prefer early exit:

if (!getLangOpts().CPlusPlus11)
  return;
<everything else>

docs/ReleaseNotes.rst

71 ↗

(On Diff #188622)

1. Please mention that this is an alias for the modernize-use-override check.
2. Please add a document for the new alias. See docs/clang-tidy/checks/cppcoreguidelines-avoid-c-arrays.rst for an example.

[alexfh]

clang-tidy/modernize/UseOverrideCheck.h

23 ↗

(On Diff #188588)

You need to override the storeOptions() method.

[alexfh]

test/clang-tidy/modernize-use-override-no-destructors.cpp

6 ↗

(On Diff #188622)

Remove the semicolons after methods. Clang-format the test (but ensure clang-format doesn't break comments at the 80 character bound).

[lewmpk]

addressed comments

[lewmpk]

thanks for the feedback everyone (and the warm welcome)

[alexfh]

Looks good! Do you need someone to commit the patch for you?

[lewmpk]

I'm trying to find a way to run the test. If someone else has already tested it then yes please.

[alexfh]

In D58731#1412831, @lewmpk wrote:

I'm trying to find a way to run the test. If someone else has already tested it then yes please.

If you've set up the build with ninja, ninja check-clang-tools should do the right thing. But I'm running the test anyway.

[alexfh]

In D58731#1412843, @alexfh wrote:

In D58731#1412831, @lewmpk wrote:

I'm trying to find a way to run the test. If someone else has already tested it then yes please.

If you've set up the build with ninja, ninja check-clang-tools should do the right thing. But I'm running the test anyway.

The new test fails even after I fixed the YAML config in the RUN lines. Please fix and re-upload the patch.

[lewmpk]

• fixed tests
• fixed typo in documentation

[Eugene.Zelenko]

docs/clang-tidy/checks/modernize-use-override.rst

5 ↗

(On Diff #188638)

Will be good idea to remove duplicated empty line.

14 ↗

(On Diff #188638)

Please use ` to highlight value.

[lewmpk]

cleaned up documentation

[MyDeveloperDay]

In D58731#1412930, @lewmpk wrote:

cleaned up documentation

Are you planning on landing this anytime soon given that it was accepted? I would like to land D57087: [clang-tidy] add OverrideMacro to modernize-use-override check but I suspect either you are I will need to rebase, I'm just trying to be nice and ask if you'd like to land first?

[lewmpk]

I'm happy to land this ASAP but I don't have commit rights

[MyDeveloperDay]

In D58731#1413476, @lewmpk wrote:

I'm happy to land this ASAP but I don't have commit rights

So one of us could land it for you.. (I've not personally done that before as I'm a bit green too! but I do have commit rights)

Its pretty easy to get commit rights, and given your looking at multiple issues I'd recommend it.. (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access)

[JonasToth]

In D58731#1413498, @MyDeveloperDay wrote:

In D58731#1413476, @lewmpk wrote:

I'm happy to land this ASAP but I don't have commit rights

So one of us could land it for you.. (I've not personally done that before as I'm a bit green too! but I do have commit rights)

Its pretty easy to get commit rights, and given your looking at multiple issues I'd recommend it.. (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access)

@MyDeveloperDay if you want to try the procedure out, go ahead ;) Otherwise I can land this patch now.

[alexfh]

In D58731#1413552, @JonasToth wrote:

In D58731#1413498, @MyDeveloperDay wrote:

In D58731#1413476, @lewmpk wrote:

I'm happy to land this ASAP but I don't have commit rights

So one of us could land it for you.. (I've not personally done that before as I'm a bit green too! but I do have commit rights)

Its pretty easy to get commit rights, and given your looking at multiple issues I'd recommend it.. (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access)

@MyDeveloperDay if you want to try the procedure out, go ahead ;) Otherwise I can land this patch now.

I'll let you folks do this. But please note that the patch wasn't generated with arcanist, so it doesn't apply with arc patch --nobranch. Copy-pasting the raw diff to patch -p0 works though (as would downloading the diff and feeding it to patch as a file). Make sure to put "Differential Revision: https://reviews.llvm.org/D58731" at the end of the message for Phabricator to close this revision automatically.

[MyDeveloperDay]

In D58731#1413695, @alexfh wrote:

In D58731#1413552, @JonasToth wrote:

In D58731#1413498, @MyDeveloperDay wrote:

In D58731#1413476, @lewmpk wrote:

I'm happy to land this ASAP but I don't have commit rights

So one of us could land it for you.. (I've not personally done that before as I'm a bit green too! but I do have commit rights)

Its pretty easy to get commit rights, and given your looking at multiple issues I'd recommend it.. (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access)

@MyDeveloperDay if you want to try the procedure out, go ahead ;) Otherwise I can land this patch now.

I'll let you folks do this. But please note that the patch wasn't generated with arcanist, so it doesn't apply with arc patch --nobranch. Copy-pasting the raw diff to patch -p0 works though (as would downloading the diff and feeding it to patch as a file). Make sure to put "Differential Revision: https://reviews.llvm.org/D58731" at the end of the message for Phabricator to close this revision automatically.

@JonasToth you are more qualified than me please go ahead...

[JonasToth]

Thank you for the patch!

[MyDeveloperDay]

@JonasToth I left a comment in the commit needed to fix the index.rst, which I don't think your later review fixes, sphinx complained about the rst file being an unreferenced octtree

https://reviews.llvm.org/rG5fbeff797a9dba504f08f14c4fa59b6f1076fe72#inline-2691

[JonasToth]

In D58731#1413888, @MyDeveloperDay wrote:

@JonasToth I left a comment in the commit needed to fix the index.rst, which I don't think your later review fixes, sphinx complained about the rst file being an unreferenced octtree

https://reviews.llvm.org/rG5fbeff797a9dba504f08f14c4fa59b6f1076fe72#inline-2691

Yup. Thank you for the hint. I was just blind and couldn't see the solution :D

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 //===--- CppCoreGuidelinesModule.cpp - clang-tidy -------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../misc/NonPrivateMemberVariablesInClassesCheck.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
 #include "../modernize/AvoidCArraysCheck.h"
+#include "../modernize/UseOverrideCheck.h"
 #include "../readability/MagicNumbersCheck.h"
 #include "AvoidGotoCheck.h"
 #include "InterfacesGlobalInitCheck.h"
 #include "MacroUsageCheck.h"
 #include "NarrowingConversionsCheck.h"
 #include "NoMallocCheck.h"
 #include "OwningMemoryCheck.h"
 #include "ProBoundsArrayToPointerDecayCheck.h"

 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<modernize::AvoidCArraysCheck>(
         "cppcoreguidelines-avoid-c-arrays");
     CheckFactories.registerCheck<AvoidGotoCheck>(
         "cppcoreguidelines-avoid-goto");
     CheckFactories.registerCheck<readability::MagicNumbersCheck>(
         "cppcoreguidelines-avoid-magic-numbers");
+    CheckFactories.registerCheck<modernize::UseOverrideCheck>(
+        "cppcoreguidelines-explicit-virtual-functions");
     CheckFactories.registerCheck<InterfacesGlobalInitCheck>(
         "cppcoreguidelines-interfaces-global-init");
     CheckFactories.registerCheck<MacroUsageCheck>(
         "cppcoreguidelines-macro-usage");
     CheckFactories.registerCheck<NarrowingConversionsCheck>(
         "cppcoreguidelines-narrowing-conversions");
     CheckFactories.registerCheck<NoMallocCheck>("cppcoreguidelines-no-malloc");
     CheckFactories.registerCheck<misc::NonPrivateMemberVariablesInClassesCheck>(
public:
 
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     ClangTidyOptions::OptionMap &Opts = Options.CheckOptions;
 
     Opts["cppcoreguidelines-non-private-member-variables-in-classes."
          "IgnoreClassesWithAllMemberVariablesBeingPublic"] = "1";
 
+    Opts["cppcoreguidelines-explicit-virtual-functions."
+         "IgnoreDestructors"] = "1";
+
     return Options;
   }
 };
 
 // Register the LLVMTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<CppCoreGuidelinesModule>
     X("cppcoreguidelines-module", "Adds checks for the C++ Core Guidelines.");
 
 } // namespace cppcoreguidelines
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the CppCoreGuidelinesModule.
 volatile int CppCoreGuidelinesModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.h

 namespace clang {
 namespace tidy {
 namespace modernize {
 
 /// Use C++11's `override` and remove `virtual` where applicable.
 class UseOverrideCheck : public ClangTidyCheck {
 public:
   UseOverrideCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+      : ClangTidyCheck(Name, Context),
+        IgnoreDestructors(Options.get("IgnoreDestructors", false)) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+  const bool IgnoreDestructors;
 };
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEOVERRIDECHECK_H

clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp

 #include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace modernize {
 
+void UseOverrideCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IgnoreDestructors", IgnoreDestructors);
+}
+
 void UseOverrideCheck::registerMatchers(MatchFinder *Finder) {
   // Only register the matcher for C++11.
-  if (getLangOpts().CPlusPlus11)
+  if (!getLangOpts().CPlusPlus11)
+    return;
+
+  if (IgnoreDestructors)
+    Finder->addMatcher(
+        cxxMethodDecl(isOverride(), unless(cxxDestructorDecl())).bind("method"),
+        this);
+  else
     Finder->addMatcher(cxxMethodDecl(isOverride()).bind("method"), this);
 }
 
 // Re-lex the tokens to get precise locations to insert 'override' and remove
 // 'virtual'.
 static SmallVector<Token, 16>
 ParseTokens(CharSourceRange Range, const MatchFinder::MatchResult &Result) {
   const SourceManager &Sources = *Result.SourceManager;

clang-tools-extra/trunk/docs/ReleaseNotes.rst

 
 - New :doc:`google-readability-avoid-underscore-in-googletest-name
   <clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name>`
   check.
 
   Checks whether there are underscores in googletest test and test case names in
   test macros, which is prohibited by the Googletest FAQ.
 
+- New alias :doc:`cppcoreguidelines-explicit-virtual-functions
+  <clang-tidy/checks/cppcoreguidelines-explicit-virtual-functions>` to
+  :doc:`modernize-use-override
+  <clang-tidy/checks/modernize-use-override>` was added.
+
 - The :doc:`bugprone-argument-comment
   <clang-tidy/checks/bugprone-argument-comment>` now supports
   `CommentBoolLiterals`, `CommentIntegerLiterals`,  `CommentFloatLiterals`,
   `CommentUserDefiniedLiterals`, `CommentStringLiterals`,
   `CommentCharacterLiterals` & `CommentNullPtrs` options.
 
 - The `Acronyms` and `IncludeDefaultAcronyms` options for the
   :doc:`objc-property-declaration <clang-tidy/checks/objc-property-declaration>`

clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-explicit-virtual-functions.rst

+.. title:: clang-tidy - cppcoreguidelines-explicit-virtual-functions
+.. meta::
+   :http-equiv=refresh: 5;URL=cppcoreguidelines-explicit-virtual-functions.html
+
+cppcoreguidelines-explicit-virtual-functions
+============================================
+
+The cppcoreguidelines-explicit-virtual-functions check is an alias, please see
+`modernize-use-override <modernize-use-override.html>`_
+for more information.

clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-override.rst

 .. title:: clang-tidy - modernize-use-override
 
 modernize-use-override
 ======================
 
-
 Use C++11's ``override`` and remove ``virtual`` where applicable.
+
+Options
+-------
+
+.. option:: IgnoreDestructors
+
+   If set to non-zero, this check will not diagnose destructors. Default is `0`.

clang-tools-extra/trunk/test/clang-tidy/modernize-use-override-no-destructors.cpp

+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-override.IgnoreDestructors, value: 1}]}" \
+// RUN: -- -std=c++11
+
+struct Base {
+  virtual ~Base();
+  virtual void f();
+};
+
+struct Simple : public Base {
+  virtual ~Simple();
+  // CHECK-MESSAGES-NOT: warning:
+  virtual void f();
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void f() override;
+};