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

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius 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>);
----------------
mstorsjo wrote:
> 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?
FYI, it is a bug in MSVC fixed in version 19.28 (2019 Update 8).
Precisely, MSVC before this version preferred integral-conversion over floating-to-integral conversion (and so far clang-cl implements the old behaviour only, cf. https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaOverload.cpp#L4109-L4125). I have a local patch for that already.

I agree to simplify this and just use `long long` (or user-defined types) instead of `long`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99641