From 128af315957eb1516c3b7ffd389774a0287ba300 Mon Sep 17 00:00:00 2001 From: Eric Fiselier Date: Fri, 12 Jul 2019 21:32:11 +0000 Subject: [PATCH] Add option to disable variant narrowing conversion changes. The paper P0608R3 - "A sane variant converting constructor" disallows narrowing conversions in variant. It was meant to address this surprising problem: std::variant v = "abc"; assert(v.index() == 1); // constructs a bool. However, it also disables every potentially narrowing conversion. For example: variant v = 0; // ill-formed variant v2 = 42; // ill-formed (int -> double narrows) These latter changes break code. A lot of code. Within Google it broke on the order of a hundred thousand target with thousands of root causes responsible for the breakages. Of the breakages related to the narrowing restrictions, none of them exposed outstanding bugs. However, the breakages caused by boolean conversions (~13 root causes), all but one of them were bugs. For this reasons, I am adding a flag to disable the narrowing conversion changes but not the boolean conversions one. One purpose of this flag is to allow users to opt-out of breaking changes in variant until the offending code can be cleaned up. For non-trivial variant usages the amount of cleanup may be significant. This flag is also required to support automated tooling, such as clang-tidy, that can automatically fix code broken by this change. In order for clang-tidy to know the correct alternative to construct, it must know what alternative was being constructed previously, which means running it over the old version of std::variant. Because this change breaks so much code, I will be implementing the aforementioned clang-tidy check in the very near future. Additionally I'm plan present this new information to the committee so they can re-consider if this is a breaking change we want to make. I think libc++ should very seriously consider pulling this change before the 9.0 release branch is cut. But that's a separate discussion that I will start on the lists. For now this is the minimal first step. llvm-svn: 365960 --- libcxx/include/variant | 4 ++ .../variant.variant/variant.assign/T.pass.cpp | 5 ++- .../variant.variant/variant.assign/conv.fail.cpp | 52 ---------------------- .../variant.variant/variant.assign/conv.pass.cpp | 43 ++++++++++++++++++ .../variant.variant/variant.ctor/T.pass.cpp | 4 +- .../variant.variant/variant.ctor/conv.fail.cpp | 39 ---------------- .../variant.variant/variant.ctor/conv.pass.cpp | 42 +++++++++++++++++ libcxx/test/support/variant_test_helpers.hpp | 9 ++++ 8 files changed, 105 insertions(+), 93 deletions(-) delete mode 100644 libcxx/test/std/utilities/variant/variant.variant/variant.assign/conv.fail.cpp create mode 100644 libcxx/test/std/utilities/variant/variant.variant/variant.assign/conv.pass.cpp delete mode 100644 libcxx/test/std/utilities/variant/variant.variant/variant.ctor/conv.fail.cpp create mode 100644 libcxx/test/std/utilities/variant/variant.variant/variant.ctor/conv.pass.cpp diff --git a/libcxx/include/variant b/libcxx/include/variant index baf1630..420e8c2 100644 --- a/libcxx/include/variant +++ b/libcxx/include/variant @@ -1103,7 +1103,11 @@ struct __overload<_Tp, _Types...> : __overload<_Types...> { template auto operator()(_Tp, _Up&& __t) const +#ifndef _LIBCPP_ENABLE_NARROWING_CONVERSIONS_IN_VARIANT -> decltype(__test({ _VSTD::forward<_Up>(__t) })); +#else + -> __identity<_Tp>; +#endif }; template diff --git a/libcxx/test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp b/libcxx/test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp index b2b53d6..ab4ea7e 100644 --- a/libcxx/test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp +++ b/libcxx/test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp @@ -136,7 +136,8 @@ void test_T_assignment_sfinae() { } { using V = std::variant; - static_assert(!std::is_assignable::value, "no matching operator="); + static_assert(std::is_assignable::value == VariantAllowsNarrowingConversions, + "no matching operator="); } { using V = std::variant, bool>; @@ -187,6 +188,7 @@ void test_T_assignment_basic() { assert(v.index() == 1); assert(std::get<1>(v) == 43); } +#ifndef TEST_VARIANT_ALLOWS_NARROWING_CONVERSIONS { std::variant v; v = 42; @@ -196,6 +198,7 @@ void test_T_assignment_basic() { assert(v.index() == 0); assert(std::get<0>(v) == 43); } +#endif { std::variant v = true; v = "bar"; diff --git a/libcxx/test/std/utilities/variant/variant.variant/variant.assign/conv.fail.cpp b/libcxx/test/std/utilities/variant/variant.variant/variant.assign/conv.fail.cpp deleted file mode 100644 index d5f370d..0000000 --- a/libcxx/test/std/utilities/variant/variant.variant/variant.assign/conv.fail.cpp +++ /dev/null @@ -1,52 +0,0 @@ -// -*- C++ -*- -//===----------------------------------------------------------------------===// -// -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. -// See https://llvm.org/LICENSE.txt for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -// -//===----------------------------------------------------------------------===// - -// UNSUPPORTED: c++98, c++03, c++11, c++14 - -// - -// template class variant; - -// template -// variant& operator=(T&&) noexcept(see below); - -#include -#include -#include - -int main(int, char**) -{ - std::variant v1; - std::variant v2; - std::variant v3; - v1 = 1; // expected-error {{no viable overloaded '='}} - v2 = 1; // expected-error {{no viable overloaded '='}} - v3 = 1; // expected-error {{no viable overloaded '='}} - - std::variant v4; - std::variant v5; - std::variant v6; - v4 = 1; // expected-error {{no viable overloaded '='}} - v5 = 1; // expected-error {{no viable overloaded '='}} - v6 = 1; // expected-error {{no viable overloaded '='}} - - std::variant v7; - std::variant v8; - std::variant v9; - v7 = "meow"; // expected-error {{no viable overloaded '='}} - v8 = "meow"; // expected-error {{no viable overloaded '='}} - v9 = "meow"; // expected-error {{no viable overloaded '='}} - - std::variant v10; - std::variant v11; - std::variant v12; - v10 = std::true_type(); // expected-error {{no viable overloaded '='}} - v11 = std::unique_ptr(); // expected-error {{no viable overloaded '='}} - v12 = nullptr; // expected-error {{no viable overloaded '='}} -} diff --git a/libcxx/test/std/utilities/variant/variant.variant/variant.assign/conv.pass.cpp b/libcxx/test/std/utilities/variant/variant.variant/variant.assign/conv.pass.cpp new file mode 100644 index 0000000..b16cf2c --- /dev/null +++ b/libcxx/test/std/utilities/variant/variant.variant/variant.assign/conv.pass.cpp @@ -0,0 +1,43 @@ +// -*- C++ -*- +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +// UNSUPPORTED: c++98, c++03, c++11, c++14 + +// + +// template class variant; + +// template +// variant& operator=(T&&) noexcept(see below); + +#include +#include +#include + +#include "variant_test_helpers.hpp" + +int main(int, char**) +{ + static_assert(!std::is_assignable, int>::value, ""); + static_assert(!std::is_assignable, int>::value, ""); + static_assert(std::is_assignable, int>::value == VariantAllowsNarrowingConversions, ""); + + static_assert(std::is_assignable, int>::value == VariantAllowsNarrowingConversions, ""); + static_assert(std::is_assignable, int>::value == VariantAllowsNarrowingConversions, ""); + static_assert(!std::is_assignable, int>::value, ""); + + static_assert(!std::is_assignable, decltype("meow")>::value, ""); + static_assert(!std::is_assignable, decltype("meow")>::value, ""); + static_assert(!std::is_assignable, decltype("meow")>::value, ""); + + static_assert(!std::is_assignable, std::true_type>::value, ""); + static_assert(!std::is_assignable, std::unique_ptr >::value, ""); + static_assert(!std::is_assignable, decltype(nullptr)>::value, ""); + +} diff --git a/libcxx/test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp b/libcxx/test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp index 40fa20b..55f8d11 100644 --- a/libcxx/test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp +++ b/libcxx/test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp @@ -69,7 +69,7 @@ void test_T_ctor_sfinae() { } { using V = std::variant; - static_assert(!std::is_constructible::value, + static_assert(std::is_constructible::value == VariantAllowsNarrowingConversions, "no matching constructor"); } { @@ -127,11 +127,13 @@ void test_T_ctor_basic() { static_assert(v.index() == 1, ""); static_assert(std::get<1>(v) == 42, ""); } +#ifndef TEST_VARIANT_ALLOWS_NARROWING_CONVERSIONS { constexpr std::variant v(42); static_assert(v.index() == 1, ""); static_assert(std::get<1>(v) == 42, ""); } +#endif { std::variant v = "foo"; assert(v.index() == 0); diff --git a/libcxx/test/std/utilities/variant/variant.variant/variant.ctor/conv.fail.cpp b/libcxx/test/std/utilities/variant/variant.variant/variant.ctor/conv.fail.cpp deleted file mode 100644 index 76b42ac..0000000 --- a/libcxx/test/std/utilities/variant/variant.variant/variant.ctor/conv.fail.cpp +++ /dev/null @@ -1,39 +0,0 @@ -// -*- C++ -*- -//===----------------------------------------------------------------------===// -// -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. -// See https://llvm.org/LICENSE.txt for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -// -//===----------------------------------------------------------------------===// - -// UNSUPPORTED: c++98, c++03, c++11, c++14 - -// - -// template class variant; - -// template constexpr variant(T&&) noexcept(see below); - -#include -#include -#include - -int main(int, char**) -{ - std::variant v1 = 1; // expected-error {{no viable conversion}} - std::variant v2 = 1; // expected-error {{no viable conversion}} - std::variant v3 = 1; // expected-error {{no viable conversion}} - - std::variant v4 = 1; // expected-error {{no viable conversion}} - std::variant v5 = 1; // expected-error {{no viable conversion}} - std::variant v6 = 1; // expected-error {{no viable conversion}} - - std::variant v7 = "meow"; // expected-error {{no viable conversion}} - std::variant v8 = "meow"; // expected-error {{no viable conversion}} - std::variant v9 = "meow"; // expected-error {{no viable conversion}} - - std::variant v10 = std::true_type(); // expected-error {{no viable conversion}} - std::variant v11 = std::unique_ptr(); // expected-error {{no viable conversion}} - std::variant v12 = nullptr; // expected-error {{no viable conversion}} -} diff --git a/libcxx/test/std/utilities/variant/variant.variant/variant.ctor/conv.pass.cpp b/libcxx/test/std/utilities/variant/variant.variant/variant.ctor/conv.pass.cpp new file mode 100644 index 0000000..4799123 --- /dev/null +++ b/libcxx/test/std/utilities/variant/variant.variant/variant.ctor/conv.pass.cpp @@ -0,0 +1,42 @@ +// -*- C++ -*- +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +// UNSUPPORTED: c++98, c++03, c++11, c++14 + +// + +// template class variant; + +// template constexpr variant(T&&) noexcept(see below); + +#include +#include +#include + +#include "variant_test_helpers.hpp" + +int main(int, char**) +{ + static_assert(!std::is_constructible, int>::value, ""); + static_assert(!std::is_constructible, int>::value, ""); + static_assert(std::is_constructible, int>::value == VariantAllowsNarrowingConversions, ""); + + static_assert(std::is_constructible, int>::value == VariantAllowsNarrowingConversions, ""); + static_assert(std::is_constructible, int>::value == VariantAllowsNarrowingConversions, ""); + static_assert(!std::is_constructible, int>::value, ""); + + static_assert(!std::is_constructible, decltype("meow")>::value, ""); + static_assert(!std::is_constructible, decltype("meow")>::value, ""); + static_assert(!std::is_constructible, decltype("meow")>::value, ""); + + static_assert(!std::is_constructible, std::true_type>::value, ""); + static_assert(!std::is_constructible, std::unique_ptr >::value, ""); + static_assert(!std::is_constructible, decltype(nullptr)>::value, ""); + +} diff --git a/libcxx/test/support/variant_test_helpers.hpp b/libcxx/test/support/variant_test_helpers.hpp index 59988b5..9d4ceec 100644 --- a/libcxx/test/support/variant_test_helpers.hpp +++ b/libcxx/test/support/variant_test_helpers.hpp @@ -21,6 +21,15 @@ // FIXME: Currently the variant tests are disabled using this macro. #define TEST_VARIANT_HAS_NO_REFERENCES +#ifdef _LIBCPP_ENABLE_NARROWING_CONVERSIONS_IN_VARIANT +# define TEST_VARIANT_ALLOWS_NARROWING_CONVERSIONS +#endif + +#ifdef TEST_VARIANT_ALLOWS_NARROWING_CONVERSIONS +constexpr bool VariantAllowsNarrowingConversions = true; +#else +constexpr bool VariantAllowsNarrowingConversions = false; +#endif #ifndef TEST_HAS_NO_EXCEPTIONS struct CopyThrows { -- 2.7.4