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