[PATCH] D111041: [clang-tidy] Remove 'IgnoreDestructors = true' from cppcoreguidelines-explicit-virtual-functions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!



================
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;
----------------
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!


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

https://reviews.llvm.org/D111041