[PATCH] D57087: [clang-tidy] add OverrideMacro to modernize-use-override check

[Alexander Kornienko via Phabricator via cfe-commits]

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