Try to get readability-deleted-default.cpp to pass on Windows.

[thakis]

Summary

In MS compatibility mode, "extern inline void g()" is not a redundant
declaration for "inline void g()", because of redeclForcesDefMSVC()
(see PR19264, r205485).

To fix, run the test with -fms-compatiblity forced on and off
and explicit check for the differing behavior for extern inline.

Final bit of PR43593.

[thakis]

There's just one other use of -fno-ms-compatibility in clang-tidy's test suite, so I'm not 100% sure this is the way to go. It's better than disabling the test completely on Windows :)

[gribozavr]

It looks to me that a better fix is to fix the checker to not emit this warning in MS compatibility mode.

I'm OK with "fixing" the test like this, however, it should come with a TODO.

[thakis]

In 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?

(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.)

[aaron.ballman]

In 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.

+1

I'm OK with "fixing" the test like this, however, it should come with a TODO.

I think we should also open a bug (or repurpose the existing one) to track the fact that the bug still exists.

[gribozavr]

There's no shortage of possible implicit TODOs

I don't see "-fno-ms-compatibility" as an implicit TODO. It is most commonly used as "this test does something that does not work in MS mode". When I read it, I can't tell why it is there. When other people write tests, they will copy-paste the RUN line into their own test.

[aaron.ballman]

In D68640#1699605, @thakis wrote:

In 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.

[thakis]

In D68640#1699628, @aaron.ballman wrote:

In D68640#1699605, @thakis wrote:

In 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.

[thakis]

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.

To answer my question: https://clang.llvm.org/extra/clang-tidy/Contributing.html#testing-checks explains how to do this. I'll update the patch.

[thakis]

make test more explicit

[aaron.ballman]

LGTM, thank you for this!

[gribozavr]

Thanks!

clang-tools-extra/test/clang-tidy/readability-redundant-declaration.cpp

 // RUN: %check_clang_tidy %s readability-redundant-declaration %t -- \
 // RUN:   -config="{CheckOptions: \
 // RUN:             [{key: readability-redundant-declaration.IgnoreMacros, \
 // RUN:               value: 0}]}"
+//
+// With -fms-compatibility and -DEXTERNINLINE, the extern inline shouldn't
+// produce additional diagnostics, so same check suffix as before:
+// RUN: %check_clang_tidy %s readability-redundant-declaration %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN:             [{key: readability-redundant-declaration.IgnoreMacros, \
+// RUN:               value: 0}]}" -- -fms-compatibility -DEXTERNINLINE
+//
+// With -fno-ms-compatiblity, DEXTERNINLINE causes additional output.
+// (The leading ',' means "default checks in addition to NOMSCOMPAT checks.)
+// RUN: %check_clang_tidy -check-suffix=,NOMSCOMPAT \
+// RUN:   %s readability-redundant-declaration %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN:             [{key: readability-redundant-declaration.IgnoreMacros, \
+// RUN:               value: 0}]}" -- -fno-ms-compatibility -DEXTERNINLINE
 
 extern int Xyz;
 extern int Xyz; // Xyz
 // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant 'Xyz' declaration [readability-redundant-declaration]
 // CHECK-FIXES: {{^}}// Xyz{{$}}
 int Xyz = 123;
 
 extern int A;

 } // namespace macros
 
 inline void g() {}
 
 inline void g(); // g
 // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: redundant 'g' declaration
 // CHECK-FIXES: {{^}}// g{{$}}
 
+#if defined(EXTERNINLINE)
 extern inline void g(); // extern g
-// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: redundant 'g' declaration
-// CHECK-FIXES: {{^}}// extern g{{$}}
+// CHECK-MESSAGES-NOMSCOMPAT: :[[@LINE-1]]:20: warning: redundant 'g' declaration
+// CHECK-FIXES-NOMSCOMPAT: {{^}}// extern g{{$}}
+#endif