[PATCH] D68640: Try to get readability-deleted-default.cpp to pass on Windows.

Nico Weber via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Oct 8 11:25:51 PDT 2019


thakis added a comment.

In D68640#1699628 <https://reviews.llvm.org/D68640#1699628>, @aaron.ballman wrote:

> In D68640#1699605 <https://reviews.llvm.org/D68640#1699605>, @thakis wrote:
>
> > In D68640#1699563 <https://reviews.llvm.org/D68640#1699563>, @gribozavr wrote:
> >
> > > It looks to me that a better fix is to fix the checker to not emit this warning in MS compatibility mode.
> >
> >
> > The warning is about emitting "here's a redundant declaration", and the test expects an "extern inline" decl to be redundant with an "inline" definition. In ms compat mode they aren't, so the checker does not emit the warning in ms mode (but does elsewhere).
> >
> > Arguably having a check that suggests removing a bunch of code that's necessary in some modes (ms compat, or C) is a bit weird, so maybe we should never emit this diag for extern inlines. I don't know which policy decisions clang-tidy usually makes for cross-platform development – does it prioritize cross-platform dev, or completeness assuming single-platform dev?
>
>
> I think it depends on the check, but for a check in the `readability` module, I'm not certain there's a clear answer. My gut feeling is to diagnose based on platform behavior because the goal is to remove redundancy and that requires platform-specific knowledge. But it's not a strong opinion.
>
> > (Finally, these tests have been broken for months, folks are landing lots of stuff with "UNSUPPORTED: win32" (clang VFS patches recently, for example) and we're struggling just keeping tests green on Windows. There's no shortage of possible implicit TODOs :) I think it's better to land this to get the check-clang-tools target green than it is to mark the test UNSUPPORTED.)
>
> To be clear, I wasn't suggesting we fix it in this patch, just that we add a FIXME/TODO to the test and make sure we're tracking the bug. Explicit TODO > implicit TODO.


Can you clarify what exactly the TODO is? As-is, the check suggests removing the redeclaration where it's a no-op (non-ms) but not where it isn't (C, ms compat). If I understand your reply correctly, this is desired behavior. Is the TODO then to have test coverage for the ms compat case? If so: Can clang-tidy checks have different CHECK-MESSAGES suffixes in the same file? If so, we can just run the test once with -fms-compat and once with -fno-ms-compat and expect the diag in one case and not in the other.


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

https://reviews.llvm.org/D68640





More information about the cfe-commits mailing list