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

Tue Oct 8 07:38:39 PDT 2019

aaron.ballman added a comment.

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.

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

https://reviews.llvm.org/D68640