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