[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

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

clang-tidy/modernize/UseOverrideCheck.h

23

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

27

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

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

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

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-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 #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"

         "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>(

     Opts["cppcoreguidelines-non-private-member-variables-in-classes."
          "IgnoreClassesWithAllMemberVariablesBeingPublic"] = "1";
 
+    Opts["cppcoreguidelines-explicit-virtual-functions."
+         "IgnoreDestructors"] = "1";
+
     return Options;
   }
 };

clang-tidy/modernize/UseOverrideCheck.h

 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;
+
+protected:
+  const bool IgnoreDestructors;
 };
 
 } // namespace modernize

clang-tidy/modernize/UseOverrideCheck.cpp

 
 void UseOverrideCheck::registerMatchers(MatchFinder *Finder) {
   // Only register the matcher for C++11.
-  if (getLangOpts().CPlusPlus11)
-    Finder->addMatcher(cxxMethodDecl(isOverride()).bind("method"), this);
+  if (getLangOpts().CPlusPlus11) {
+    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