Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 19 commits into from
Feb 18, 2021
Merged

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

merged 19 commits into from
Feb 18, 2021

Conversation

MichaelRizkalla
Copy link
Contributor

@MichaelRizkalla MichaelRizkalla commented Feb 6, 2021

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)

This is a squash of multiple commits for readability
Changes according to P1957R2
Adjust variant type selection templates, add unit tests
@MichaelRizkalla MichaelRizkalla requested a review from a team as a code owner February 6, 2021 02:18
stl/inc/variant Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added the cxx20 C++20 feature label Feb 6, 2021
- Also propagate the type as R-value correctly

Co-Authored-By: Igor Zhukov <4289847+fsb4000@users.noreply.github.com>
Copy link
Contributor

@fsb4000 fsb4000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fsb4000
Copy link
Contributor

fsb4000 commented Feb 6, 2021

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 and others added 2 commits February 6, 2021 13:42
Co-Authored-By: Igor Zhukov <4289847+fsb4000@users.noreply.github.com>
@MichaelRizkalla
Copy link
Contributor Author

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
Copy link
Contributor

fsb4000 commented Feb 8, 2021

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
Copy link
Contributor

fsb4000 commented Feb 10, 2021

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
Copy link
Contributor Author

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 🆘.

@CaseyCarter CaseyCarter self-assigned this Feb 10, 2021
@fsb4000
Copy link
Contributor

fsb4000 commented Feb 11, 2021

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
Copy link
Contributor

fsb4000 commented Feb 11, 2021

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

@MichaelRizkalla
Copy link
Contributor Author

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
Copy link
Contributor

fsb4000 commented Feb 11, 2021

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...

This commit re-disables libc++ tests and guards tests which exhibit a behavior where narrowing conversion in user-defined type constructor is allowed by clang-cl and disallowed by cl. I assume cl is correct with this commit.

Co-Authored-By: Igor Zhukov <4289847+fsb4000@users.noreply.github.com>
@MichaelRizkalla
Copy link
Contributor Author

MichaelRizkalla commented Feb 11, 2021

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
Copy link
Contributor

fsb4000 commented Feb 11, 2021

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

stl/inc/variant Outdated Show resolved Hide resolved
MichaelRizkalla and others added 2 commits February 12, 2021 00:02
Co-Authored-By: S. B. Tam <8998201+cpplearner@users.noreply.github.com>
* Guard failing tests on EDG

Co-Authored-By: Michael S. Rizkalla <Michael.Anis1995@gmail.com>
tests/std/test.lst Outdated Show resolved Hide resolved
stl/inc/variant Show resolved Hide resolved
stl/inc/variant Outdated Show resolved Hide resolved
stl/inc/variant Outdated Show resolved Hide resolved
stl/inc/variant Outdated Show resolved Hide resolved
@StephanTLavavej

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@fsb4000
Copy link
Contributor

fsb4000 commented Feb 12, 2021

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 StephanTLavavej removed their assignment Feb 15, 2021
@StephanTLavavej
Copy link
Member

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.

Casey suggested _TargetType and _InitializerType to avoid confusion.

constexpr is unnecessary for _Construct_array.

typename/template keywords aren't necessary as _Variant_type_test isn't templated.
@StephanTLavavej
Copy link
Member

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
Copy link
Member

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.

Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a metaprogramming simplification; looks good otherwise.

stl/inc/variant Outdated Show resolved Hide resolved
StephanTLavavej and others added 2 commits February 17, 2021 15:54
Co-authored-by: Casey Carter <cacarter@microsoft.com>
@StephanTLavavej StephanTLavavej merged commit 87152f4 into microsoft:main Feb 18, 2021
@StephanTLavavej
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx20 C++20 feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P0608R3 Improving variant's Converting Constructor/Assignment
5 participants