From fe6f1dd5661ce3e41321094f95a97da981b85600 Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Wed, 18 Sep 2024 21:53:44 -0700 Subject: [PATCH 1/2] Short-circuit some `variant` constraints `variant`'s converting constructor and assignment operator templates are constrained to reject arguments of the `variant`'s type. In such a case, the templates instantiated to check the constructibility constraint might be ill-formed outside the immediate context of template instantiation causing a hard error. We should split the constraints into multiple `enable_if_t`s to enable short-circuiting of later constraints when the earlier constraints fail. Fixes #4959. --- stl/inc/variant | 19 ++++++------- tests/std/tests/P0088R3_variant_msvc/test.cpp | 28 +++++++++++++++++++ 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/stl/inc/variant b/stl/inc/variant index ef2a1bf9a8..c2b916ed25 100644 --- a/stl/inc/variant +++ b/stl/inc/variant @@ -934,12 +934,11 @@ public: : _Mybase(in_place_index<0>) {} // value-initialize alternative 0 template , variant> // - && !_Is_specialization_v<_Remove_cvref_t<_Ty>, in_place_type_t> // - && !_Is_in_place_index_specialization<_Remove_cvref_t<_Ty>> // - && is_constructible_v<_Variant_init_type<_Ty, _Types...>, _Ty>, // - int> = 0> + enable_if_t, variant> + && !_Is_specialization_v<_Remove_cvref_t<_Ty>, in_place_type_t> + && !_Is_in_place_index_specialization<_Remove_cvref_t<_Ty>>, + int> = 0, + enable_if_t, _Ty>, int> = 0> constexpr variant(_Ty&& _Obj) noexcept(is_nothrow_constructible_v<_Variant_init_type<_Ty, _Types...>, _Ty>) : _Mybase(in_place_index<_Variant_init_index<_Ty, _Types...>::value>, static_cast<_Ty&&>(_Obj)) { // initialize to the type selected by passing _Obj to the overload set f(Types)... @@ -976,10 +975,10 @@ public: // initialize alternative _Idx from _Ilist and _Args... } - template , variant> - && is_constructible_v<_Variant_init_type<_Ty, _Types...>, _Ty> - && is_assignable_v<_Variant_init_type<_Ty, _Types...>&, _Ty>, - int> = 0> + template , variant>, int> = 0, + enable_if_t, _Ty> + && is_assignable_v<_Variant_init_type<_Ty, _Types...>&, _Ty>, + int> = 0> _CONSTEXPR20 variant& operator=(_Ty&& _Obj) noexcept(is_nothrow_assignable_v<_Variant_init_type<_Ty, _Types...>&, _Ty> && is_nothrow_constructible_v<_Variant_init_type<_Ty, _Types...>, _Ty>) { diff --git a/tests/std/tests/P0088R3_variant_msvc/test.cpp b/tests/std/tests/P0088R3_variant_msvc/test.cpp index 412d93d68e..f564598671 100644 --- a/tests/std/tests/P0088R3_variant_msvc/test.cpp +++ b/tests/std/tests/P0088R3_variant_msvc/test.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -909,6 +910,33 @@ namespace msvc { } } } // namespace assign_cv + + namespace gh4959 { + // Test GH-4959 "P0608R3 breaks flang build with Clang" + // Constraints on variant's converting constructor and assignment operator templates reject arguments of the + // variant's type, but did not short-circuit to avoid evaluating the constructibility constraint. For this + // program, the constructibility constraint is ill-formed outside the immediate context when determing if + // variant> can be initialized from an rvalue of the same type. + + template + using NoLvalue = std::enable_if_t<(... && !std::is_lvalue_reference_v)>; + + struct Name {}; + + struct GenericSpec { + template > + GenericSpec(A&& x) : u(std::move(x)) {} + GenericSpec(GenericSpec&&) = default; + std::variant u; + }; + + struct InterfaceStmt { + template > + InterfaceStmt(A&& x) : u(std::move(x)) {} + InterfaceStmt(InterfaceStmt&&) = default; + std::variant> u; + }; + } // namespace gh4959 } // namespace msvc int main() { From 3c5aa0ad9e0e4e7f46e62cb981399f1e0393e61d Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Fri, 20 Sep 2024 10:43:01 -0700 Subject: [PATCH 2/2] STL's review comments --- stl/inc/variant | 6 +++++- tests/std/tests/P0088R3_variant_msvc/test.cpp | 6 +++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/stl/inc/variant b/stl/inc/variant index c2b916ed25..ac38081fe0 100644 --- a/stl/inc/variant +++ b/stl/inc/variant @@ -937,7 +937,9 @@ public: enable_if_t, variant> && !_Is_specialization_v<_Remove_cvref_t<_Ty>, in_place_type_t> && !_Is_in_place_index_specialization<_Remove_cvref_t<_Ty>>, - int> = 0, + int> = 0, + // These enable_if_t constraints are distinct to enable short-circuiting and avoid + // substitution into _Variant_init_type when the first constraint is not satisfied. enable_if_t, _Ty>, int> = 0> constexpr variant(_Ty&& _Obj) noexcept(is_nothrow_constructible_v<_Variant_init_type<_Ty, _Types...>, _Ty>) : _Mybase(in_place_index<_Variant_init_index<_Ty, _Types...>::value>, static_cast<_Ty&&>(_Obj)) { @@ -976,6 +978,8 @@ public: } template , variant>, int> = 0, + // These enable_if_t constraints are distinct to enable short-circuiting and avoid + // substitution into _Variant_init_type when the first constraint is not satisfied. enable_if_t, _Ty> && is_assignable_v<_Variant_init_type<_Ty, _Types...>&, _Ty>, int> = 0> diff --git a/tests/std/tests/P0088R3_variant_msvc/test.cpp b/tests/std/tests/P0088R3_variant_msvc/test.cpp index f564598671..81fadf0855 100644 --- a/tests/std/tests/P0088R3_variant_msvc/test.cpp +++ b/tests/std/tests/P0088R3_variant_msvc/test.cpp @@ -915,11 +915,11 @@ namespace msvc { // Test GH-4959 "P0608R3 breaks flang build with Clang" // Constraints on variant's converting constructor and assignment operator templates reject arguments of the // variant's type, but did not short-circuit to avoid evaluating the constructibility constraint. For this - // program, the constructibility constraint is ill-formed outside the immediate context when determing if + // program, the constructibility constraint is ill-formed outside the immediate context when determining if // variant> can be initialized from an rvalue of the same type. - template - using NoLvalue = std::enable_if_t<(... && !std::is_lvalue_reference_v)>; + template + using NoLvalue = std::enable_if_t<(... && !std::is_lvalue_reference_v)>; struct Name {};