[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>);
----------------
cjdb wrote:
> curdeius wrote:
> > 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`.
> I would prefer that we keep the existing test and guard it against the offending versions. Part of the way this test has been designed is to flush out strange compiler bugs, so this test has done its job. Per [[ http://eel.is/c++draft/tab:over.ics.scs | tab:over.ics.scs ]], this is an ambiguity, and compilers that don't follow this table are buggy because `long` is simultaneously being converted to `int` and `double`, and those conversions are equally ranked.
> 
> Here's what I'd do:
> 
> * in this patch:
>  * fix up the comment
>  * add include guards around the offending lines for the non-conforming compiler versions
>  * add a test for `long long` or `short` (or both)
> * file a bug with the respective compiler teams and ask them to adjust this patch when they make the fix (or at least notify you to do so)
> 
I can make the comment a bit more precise with wordings from @curdeius comments.

For guarding the exact version that has got the problem, it gets a bit messy, as we have MSVC proper having the bug in older versions, and Clang imitating MSVC currently always having the bug, and in the future will have it conditionally depending on what version of MSVC it impersonates:
`#if !defined(_MSC_VER) || ((!defined(__clang__) || __clang_major__ >= 13) && _MSC_VER >= 1928)`
(That becomes valid once the Clang fix lands - I'd assume it gets fixed by the time that 13 branches off.) That doesn't take Apple Clang versions into account though - they can technically be used to target MSVC as well (although I guess it's rather rare).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99641