[PATCH] D58731: [clang-tidy] added cppcoreguidelines-explicit-virtual-functions
Jonas Toth via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 27 12:01:08 PST 2019
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
More information about the cfe-commits
mailing list