[libcxx-commits] [PATCH] D99641: [libcxx] [test] Fix invocable tests on Windows

[Christopher Di Bella via Phabricator via libcxx-commits]

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

In D99641#2665140 <https://reviews.llvm.org/D99641#2665140>, @mstorsjo wrote:

> In D99641#2665097 <https://reviews.llvm.org/D99641#2665097>, @ldionne wrote:
>
>> In the test suite, we generally try to stay divorced from compiler-specific behavior if we can, the reason being that things otherwise become quite complicated. Furthermore, we're trying to test the library after all, not the language implementation (since there's another test suite for that). I do agree that the boundary between those two things (low-level library stuff and compiler stuff) is sometimes vague, for example when testing type traits that are all implemented as intrinsics.
>>
>> However, in this instance, it seems to me that we're really trying to test whether the pure library implementation does the right thing. If I had written those tests from the start, I might have gone for something like what Arthur suggested above, which IMO more clearly indicates what we're trying to test:
>>
>>   struct multiple_overloads {
>>       struct A {};
>>       struct B { B(int); };
>>       struct AB : A, B {};
>>       struct O {};
>>       void operator()(A) {};
>>       void operator()(B) {};
>>   };
>>   static_assert(std::invocable<multiple_overloads, multiple_overloads::A>);
>>   static_assert(std::invocable<multiple_overloads, multiple_overloads::B>);
>>   static_assert(std::invocable<multiple_overloads, int>);
>>   static_assert(!std::invocable<multiple_overloads, multiple_overloads::AB>);
>>   static_assert(!std::invocable<multiple_overloads, multiple_overloads::O>);
>>
>> On the other hand, I also agree with @cjdb that this test has been useful as-is: it has taught us that MSVC (and Clang in MS mode) doesn't work properly w.r.t. overload resolution. However, I believe the better location for testing that overload resolution bug would be to add (roughly) the same test to the Clang test suite (which it seems is being done). By doing that, we do weaken the libc++ test suite w.r.t. language conformance, but we don't weaken it w.r.t. library conformance.
>>
>> That's just my .02, but in all cases I would rather not just hide the issue by using `long long`.
>
> Thanks for weighing in!
>
> @cjdb - do you want to commandeer this review and finish it in the form you prefer, based on the comments above?

I think @ldionne's comments offer direction for this patch and there's terribly little I can achieve by commandeering this patch (as opposed to just reviewing it). I think the suggested code can be applied verbatim.
I suggest cross-referencing this review in the Clang review that Louis alluded to, so that we can make sure the coverage isn't lost; only transferred.

Following that, my LGTM is trivial.


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

https://reviews.llvm.org/D99641