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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.


================
Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:32
+    : ClangTidyCheck(Name, Context),
+      OverrideMacro(Options.get("OverrideMacro", "override")),
+      FinalMacro(Options.get("FinalMacro", "final")) {}
----------------
alexfh wrote:
> 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`).
I agree that is a better naming


================
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
----------------
alexfh wrote:
> "modernize-use-nodiscard"? Does this test pass?
my bad I was having problems with getting lit to work in windows environment, so was running the tests externally, I've fixed that now


```
c:/clang/llvm/build/RelWithDebInfo/bin/llvm-lit.py -v test/clang-tidy/modernize-use-override*
-- Testing: 5 tests, 5 threads --
PASS: Clang Tools :: clang-tidy/modernize-use-override-cxx98.cpp (1 of 5)
PASS: Clang Tools :: clang-tidy/modernize-use-override-ms.cpp (2 of 5)
PASS: Clang Tools :: clang-tidy/modernize-use-override-with-no-macro-inscope.cpp (3 of 5)
PASS: Clang Tools :: clang-tidy/modernize-use-override.cpp (4 of 5)
PASS: Clang Tools :: clang-tidy/modernize-use-override-with-macro.cpp (5 of 5)
Testing Time: 5.13s
  Expected Passes    : 5
```




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

https://reviews.llvm.org/D57087