alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.
Herald added a subscriber: jdoerfert.
================
Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:32
+ : ClangTidyCheck(Name, Context),
+ OverrideMacro(Options.get("OverrideMacro", "override")),
+ FinalMacro(Options.get("FinalMacro", "final")) {}
----------------
MyDeveloperDay wrote:
> alexfh wrote:
> > I'd suggest to default to an empty string and use `override` as a fallback
> > right in the code where the diagnostic is generated.
> So I tried this and and met with some issues with the unit tests where it
> seemed to think "override" was a macro, I also found myself just simply
> always setting OverrideMacro/Final Macro to "override" and "final" anyway..
> I've changed this round a little to only check for the macro when the
> OverrideMacro isn't override. This seems to resolve the problem, let me know
> if it still feels wrong.
In case "override" is not a macro, setting `OverrideMacro` to `override` would
be somewhat confusing. We could make set default to `override`, if this makes
logic simpler, but then I'd suggest to rename the option to `OverrideSpelling`
(same for `final`).
================
Comment at: docs/clang-tidy/checks/modernize-use-override.rst:9
-Use C++11's ``override`` and remove ``virtual`` where applicable.
+virtual on non base class implementations was used to help indiciate to the
user
+that a function was virtual. C++ compilers did not use the presence of this to
----------------
Please enclose "virtual" in double backquotes.
================
Comment at: docs/clang-tidy/checks/modernize-use-override.rst:13
+
+In C++ 11 ``override`` and ``final`` were introduced to allow overridden
+functions to be marked appropriately. There presence allows compilers to verify
----------------
`override` and `final` keywords were introduced ...
================
Comment at: docs/clang-tidy/checks/modernize-use-override.rst:14
+In C++ 11 ``override`` and ``final`` were introduced to allow overridden
+functions to be marked appropriately. There presence allows compilers to verify
+that an overridden function correctly overrides a base class implementation.
----------------
s/There/Their/
================
Comment at: docs/clang-tidy/checks/modernize-use-override.rst:39
+
+ For more information on the use of override see
https://en.cppreference.com/w/cpp/language/override
----------------
Enclose `override` in double backquotes.
================
Comment at: test/clang-tidy/modernize-use-override-with-macro.cpp:2
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN: -config="{CheckOptions: [{key:
modernize-use-nodiscard.OverrideMacro, value: 'OVERRIDE'},{key:
modernize-use-nodiscard.FinalMacro, value: 'FINAL'}]}" \
+// RUN: -- -std=c++11
----------------
"modernize-use-nodiscard"? Does this test pass?
================
Comment at: test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp:2
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN: -config="{CheckOptions: [{key:
modernize-use-nodiscard.OverrideMacro, value: 'CUSTOM_OVERRIDE'},{key:
modernize-use-nodiscard.FinalMacro, value: 'FINAL'}]}" \
+// RUN: -- -std=c++11
----------------
Same here.
================
Comment at: test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp:16
+public:
+ virtual ~SimpleCases();
+ // CHECK-FIXES: {{^}} virtual ~SimpleCases();
----------------
The check should still issue a warning here, imo. Maybe without a fix, but it
should draw attention to the issue.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57087/new/
https://reviews.llvm.org/D57087
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
