carlosgalvezp added a comment.
I realized I can speed up the test by removing the extra `RUN` line and simply
add the new check to the list of checks, let me fix that before merge :)
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/modernize-use-override.cpp:56
virtual ~SimpleCases();
- // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using 'override' or
(rarely) 'final' instead of 'virtual' [modernize-use-override]
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using 'override' or
(rarely) 'final' instead of 'virtual'
// CHECK-FIXES: {{^}} ~SimpleCases() override;
----------------
aaron.ballman wrote:
> carlosgalvezp wrote:
> > aaron.ballman wrote:
> > > This isn't quite what I was after; now it looks like we expect to always
> > > get the diagnostic (in fact, I'm a bit worried that this test is
> > > passing). I'd rather see:
> > > ```
> > > // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using 'override' or
> > > (rarely) 'final' instead of 'virtual' [modernize-use-override]
> > > // CHECK-CPPCOREGUIDELINES-NOT: :[[@LINE-2]]:11: warning: prefer using
> > > 'override' or (rarely) 'final' instead of 'virtual'
> > > ```
> > > So that we check explicitly we see the diagnostic for modernize and
> > > explicitly that we don't see the diagnostic for C++ Core Guidelines.
> > >
> > > You'll need to change the second RUN line to not use `check_clang_tidy`
> > > but instead execute clang-tidy manually so you can pass
> > > `-check-prefix=CHECK-CPPCOREGUIDELINES` to it (as done in
> > > https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/checkers/cert-static-object-exception.cpp#L4).
> > Not sure I follow. What you propose is something that would make sense
> > _before_ this patch, when the `cppcoreguidelines` check was _different_
> > than `modernize`. But now they are not, they are identical, so we expect to
> > get identical diagnostics from them. Or am I missing something obvious?
> > Or am I missing something obvious?
>
> No, you just caught me having a major think-o. Again. Because this is the
> second time I've gotten the changes exactly backwards in my mind. :-D Sorry
> for the confusion!
No worries! :)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111041/new/
https://reviews.llvm.org/D111041
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
