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

Christopher Di Bella via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Wed Mar 31 08:53:18 PDT 2021

cjdb accepted this revision.
cjdb 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>);
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)

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list