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

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/test/std/concepts/concepts.callable/concept.invocable/invocable.compile.pass.cpp:235
+// mimics the old MSVC behaviour where this assert fails, regardless of what
+// version of MSVC it targets.
 static_assert(!std::invocable<multiple_overloads&, long>);
----------------
Quuxplusone wrote:
> This is not "comments about the MSVC and Clang bugs that this works around": this comment doesn't explain the bug at all! It just says that the code doesn't work (for some reason, presumably a bug, but we don't say what the bug is or why).
> 
> We either need to dig in and diagnose //why// MSVC is treating this overload resolution as unambiguous...
> 
> ...or, vastly simpler IMO, notice that the point of this test case has nothing to do with `long` and `int`! The point of this test case is to verify that overload resolution ambiguity makes the invocation fail. Apparently "`long`, passed to an overload set of `int` and `double`" is //not// an ambiguity on all supported compilers. Therefore, let's use `long long` instead. (Which you do on line 238.) Cool, that fixes the test issue. So delete lines 232–237; they are no longer needed.
> 
> Even better, btw, would be to use some user-defined types whose conversions we control; e.g.
> ```
> 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>);
> ```
> These 13 lines accomplish the same stuff as the original 15, without "cross-polluting" with the (apparently non-portable) details of arithmetic conversions.
Thanks, that sounds sensible to me. @cjdb - which way forward do you prefer?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99641