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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
