<variant>: P0608R3 Improving variant's converting constructor/assignment

[MichaelRizkalla]

This PR implements P0608R3. if successful will close #27.

Locally, one test fails with "clang-cl":
That's an inconsistency between MSVC and clang-cl, and I think MSVC is correct. (assuming I am doing everything in a correct way)

[fsb4000]

There are disabled libc++ tests.

https://github.com/microsoft/STL/blob/main/tests/libcxx/expected_results.txt#L302-L306

and

https://github.com/microsoft/STL/blob/main/tests/libcxx/skipped_tests.txt#L302-L306

Just remove the lines and add the change to your pull request.

[fsb4000]

By the way, gcc and clang failed with 3 asserts:

static_assert(is_constructible_v<variant<bool>, convertible_bool>);

static_assert(!is_constructible_v<variant<double_double>, int>);

variant<float, bool> f = convertible_bool{true}; // bool
assert(f.index() == 1);
assert(get<1>(f));

https://gcc.godbolt.org/z/cT3f4r

[MichaelRizkalla]

By the way, gcc and clang failed with 3 asserts:

static_assert(is_constructible_v<variant<bool>, convertible_bool>);

static_assert(!is_constructible_v<variant<double_double>, int>);

variant<float, bool> f = convertible_bool{true}; // bool
assert(f.index() == 1);
assert(get<1>(f));

https://gcc.godbolt.org/z/cT3f4r

These are my thoughts regarding these differences:

• I think this:

static_assert(is_constructible_v<variant<bool>, convertible_bool>);

should be valid because convertible_bool implements operator bool:

Also, looking at std::variant<bool, int> a = convertible_bool{false}; here: https://gcc.godbolt.org/z/fb5hsY
That's an incorrect behaviour that was fixed in WG21-P1957

• Constructing double_double from int is a narrowing conversion, because double_double only implements double_double(double) constructor. I think it should be invalid unless double_double is to support converting from int by implementing double_double(int).

static_assert(!is_constructible_v<variant<double_double>, int>);

I would like to know your input, and maintainers' input as well 🐱.

[fsb4000]

Yes, I think you're right.

By the way. I found that clang don't implement P1957R2(DR: Converting from T* to bool should be considered narrowing) for now. So the tests are expected to be failed.

And gcc(trunc) fix the issue: https://gcc.godbolt.org/z/brMhqx

[fsb4000]

So then you will have time could you add ifndef __clang__ to failed assert with clang-cl?

Similar to the lines: https://github.com/microsoft/STL/blob/main/tests/std/tests/P0466R5_layout_compatibility_and_pointer_interconvertibility_traits/test.cpp#L18

Next we can file the failed asserts here: https://bugs.llvm.org/ and add TRANSITION comment

[MichaelRizkalla]

So then you will have time could you add ifndef __clang__ to failed assert with clang-cl?

Similar to the lines: https://github.com/microsoft/STL/blob/main/tests/std/tests/P0466R5_layout_compatibility_and_pointer_interconvertibility_traits/test.cpp#L18

Next we can file the failed asserts here: https://bugs.llvm.org/ and add TRANSITION comment

I will make sure the unit tests are passing with ifndef __clang__. I think I should disable the libc++ tests again as well, right?

Also, I would like to re-visit my second point I mentioned earlier in the discussion about:

static_assert(!is_constructible_v<variant<double_double>, int>);

I found out msvc and gcc results are different, I took out the code I am using to build Ti x[] = {std::forward<T>(t)} and tried it on both compilers, they output different results: https://godbolt.org/z/33641K
It does not make sense to me, so I'd like some help determining where the issue is 🆘.

[fsb4000]

libc++ tests failed, yes. I will try to figure out why...

C:\Dev\STL\out\build\x64>python tests\utils\stl-lit\stl-lit.py ..\..\..\llvm-project\libcxx\test\std\utilities\variant\variant.variant\

...

Failed Tests (4):
  libc++ :: std/utilities/variant/variant.variant/variant.assign/T.pass.cpp:0
  libc++ :: std/utilities/variant/variant.variant/variant.assign/conv.pass.cpp:0
  libc++ :: std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp:0
  libc++ :: std/utilities/variant/variant.variant/variant.ctor/conv.pass.cpp:0

[fsb4000]

I wrote the failed libc++ assertions: https://gcc.godbolt.org/z/nY7a4P

[MichaelRizkalla]

I wrote the failed libc++ assertions: https://gcc.godbolt.org/z/nY7a4P

std::true_type and std::false_type both implement operator bool(), therefore, they should construct the bool variant (gcc trunk is doing that already as you said earlier 🐱)

[fsb4000]

With trunk versions of compilers the assertions fail too: https://gcc.godbolt.org/z/599fG9 (with clang + libstdc++ too)

So you're correct, just disable the libc++ tests...

[MichaelRizkalla]

Notes about the one remaining failure:

• I ran it locally python tests\utils\stl-lit\stl-lit.py ..\..\..\tests\std\tests\P0088R3_variant, and it passes (that's why I pushed the last commit anyway).
• I tried the same command-line arguments from the azure failure page, it failed, and worked when I removed the "/BE" flag

[fsb4000]

Great. /BE is EDG compiler. I added #ifndef __EDG__

I can't add the changes here but I created pull request: https://github.com/MichaelRizkalla/STL/pull/2

[fsb4000]

I rerun the EDG tests after commit "Make sure base class members are visible" (2d4c87b) without #ifndef __EDG__ and I got the errors:

C:\Dev\STL\playground>cl /c /BE /std:c++latest main.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.28.29828 for x86
Copyright (C) Microsoft Corporation.  All rights reserved.

/std:c++latest is provided as a preview of language features from the latest C++
working draft, and we're eager to hear about bugs and suggestions for improvements.
However, note that these features are provided as-is without support, and subject
to changes or removal as the working draft evolves. See
https://go.microsoft.com/fwlink/?linkid=2045807 for details.

main.cpp
"main.cpp", line 39: error: static assertion failed
  static_assert(is_constructible_v<variant<float, long int>, int>);
  ^

"main.cpp", line 40: error: static assertion failed
  static_assert(is_constructible_v<variant<float, long long int>, int>);
  ^

"main.cpp", line 41: error: static assertion failed
  static_assert(is_constructible_v<variant<float, long, double>, int>);
  ^

"main.cpp", line 42: error: static assertion failed
  static_assert(is_constructible_v<variant<float, vector<int>, long long int>, int>);
  ^

"main.cpp", line 45: error: static assertion failed
  static_assert(!is_constructible_v<variant<float>, int>);
  ^

"main.cpp", line 46: error: static assertion failed
  static_assert(!is_constructible_v<variant<float, vector<int>>, int>);
  ^

"main.cpp", line 67: error: static assertion failed
  static_assert(!is_constructible_v<variant<double_double>, int>);
  ^

"main.cpp", line 69: error: static assertion failed
  static_assert(!is_constructible_v<variant<float>, unsigned int>);
  ^

"main.cpp", line 96: error: no operator "=" matches these operands
            operand types are: T2 = int
      e = 0; // long
        ^

"main.cpp", line 102: error: no suitable constructor exists to convert from
          "int" to "std::variant<float, long>"
      variant<float, long> g = 0; // long
                               ^

"main.cpp", line 105: error: no suitable constructor exists to convert from
          "int" to "std::variant<float, long, double>"
      variant<float, long, double> h = 0; // long
                                       ^

"main.cpp", line 108: error: no suitable constructor exists to convert from
          "int" to "std::variant<float, std::vector<int, std::allocator<int>>,
          long long>"
      variant<float, vector<int>, long long int> i = 0; // long long int

So #ifndef __EDG__ are still needed. EDG is complaining about something else or is it a compiler bug...

[StephanTLavavej]

I think we may need to make this conditional on C++20. That's the default for all new features - unconditional implementation is special, and reserved for invasive changes to existing machinery that are non-disruptive. (The enable_shared_from_this overhaul is a perfect example; highly invasive but doesn't break existing code.)

At first, I thought this satisfied those criteria. However, after seeing a source break in the compiler's own code where it was narrowing uint64_t to int64_t, I think the potential for disruption is higher than I first expected, and it would be relatively simple to make the constraint vary on 17/20 because the changes are so centralized (:smile_cat:). Finally, no customers will complain if we ship a feature guarded for 20 only, whereas there is a nonzero risk if we ship something that affects /std:c++17.

I'll investigate making this change.

[StephanTLavavej]

I pushed the following changes:

• Guard the feature for C++20.
  • I felt it was clearest to provide two versions of _Variant_init_single_overload::operator(), instead of making _Variant_type_resolver's definition vary.
• Dropped _FTy<_Ty> and directly used _Variant_type_resolver<_Idx, _Type, _Ty> as the C++20 return type.
  • In the original code, an _FTy typedef greatly simplified the conversion operator to function pointer, but now there are no function pointers.
• While reviewing this, I noticed that _Type and _Ty were confusingly similar; Casey suggested _TargetType and _InitializerType. I've applied this renaming (and I believe I didn't mix them up) to improve readability.
• Removed unnecessary keywords:
  • constexpr is unnecessary for _Construct_array.
  • typename/template keywords aren't necessary as _Variant_type_test isn't templated.

[StephanTLavavej]

Mirrored to internal MSVC-PR-304537 with changes to fix the compiler's own usage of variant, and skip tests affected by internal VSO-1279029 "/experimental:module assertion in BuildIdent() with C++20 variant".

Please note: Further changes are still possible (e.g. if you notice something, or if final review leads to more feedback that should be addressed before merging), but they must be replicated to the mirror PR in order to avoid codebase divergence.

[CaseyCarter]

Just a metaprogramming simplification; looks good otherwise.

[StephanTLavavej]

Thanks for implementing C++20's variant of these constraints, helping users avoid unexpected results! 😹 🚀 🎉