[PATCH] D58731: [clang-tidy] added cppcoreguidelines-explicit-virtual-functions

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

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.



================
Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:23
+  if (getLangOpts().CPlusPlus11) {
+    if (IgnoreDestructors) {
+      Finder->addMatcher(
----------------
Please ellide the braces for the single-statements (this `if` and the `else` branch)


================
Comment at: clang-tidy/modernize/UseOverrideCheck.h:23
+      : ClangTidyCheck(Name, Context),
+        IgnoreDestructors(Options.get("IgnoreDestructors", false)) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
----------------
That requires additional methods for the function for the configuration to function properly. Please see other checks and implement that the same way.


================
Comment at: clang-tidy/modernize/UseOverrideCheck.h:27
+
+protected:
+  const bool IgnoreDestructors;
----------------
please use private instead


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58731/new/

https://reviews.llvm.org/D58731