Skip to content

Commit 128af31

Browse files
committed
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<std::string, bool> v = "abc"; assert(v.index() == 1); // constructs a bool. However, it also disables every potentially narrowing conversion. For example: variant<unsigned> v = 0; // ill-formed variant<string, double> 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
1 parent 9f0d718 commit 128af31

File tree

8 files changed

+105
-93
lines changed

8 files changed

+105
-93
lines changed

libcxx/include/variant

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1103,7 +1103,11 @@ struct __overload<_Tp, _Types...> : __overload<_Types...> {
11031103

11041104
template <class _Up>
11051105
auto operator()(_Tp, _Up&& __t) const
1106+
#ifndef _LIBCPP_ENABLE_NARROWING_CONVERSIONS_IN_VARIANT
11061107
-> decltype(__test({ _VSTD::forward<_Up>(__t) }));
1108+
#else
1109+
-> __identity<_Tp>;
1110+
#endif
11071111
};
11081112

11091113
template <class _Base, class _Tp>

libcxx/test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,8 @@ void test_T_assignment_sfinae() {
136136
}
137137
{
138138
using V = std::variant<std::string, float>;
139-
static_assert(!std::is_assignable<V, int>::value, "no matching operator=");
139+
static_assert(std::is_assignable<V, int>::value == VariantAllowsNarrowingConversions,
140+
"no matching operator=");
140141
}
141142
{
142143
using V = std::variant<std::unique_ptr<int>, bool>;
@@ -187,6 +188,7 @@ void test_T_assignment_basic() {
187188
assert(v.index() == 1);
188189
assert(std::get<1>(v) == 43);
189190
}
191+
#ifndef TEST_VARIANT_ALLOWS_NARROWING_CONVERSIONS
190192
{
191193
std::variant<unsigned, long> v;
192194
v = 42;
@@ -196,6 +198,7 @@ void test_T_assignment_basic() {
196198
assert(v.index() == 0);
197199
assert(std::get<0>(v) == 43);
198200
}
201+
#endif
199202
{
200203
std::variant<std::string, bool> v = true;
201204
v = "bar";

libcxx/test/std/utilities/variant/variant.variant/variant.assign/conv.fail.cpp

Lines changed: 0 additions & 52 deletions
This file was deleted.
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
// -*- C++ -*-
2+
//===----------------------------------------------------------------------===//
3+
//
4+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
5+
// See https://llvm.org/LICENSE.txt for license information.
6+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
7+
//
8+
//===----------------------------------------------------------------------===//
9+
10+
// UNSUPPORTED: c++98, c++03, c++11, c++14
11+
12+
// <variant>
13+
14+
// template <class ...Types> class variant;
15+
16+
// template <class T>
17+
// variant& operator=(T&&) noexcept(see below);
18+
19+
#include <variant>
20+
#include <string>
21+
#include <memory>
22+
23+
#include "variant_test_helpers.hpp"
24+
25+
int main(int, char**)
26+
{
27+
static_assert(!std::is_assignable<std::variant<int, int>, int>::value, "");
28+
static_assert(!std::is_assignable<std::variant<long, long long>, int>::value, "");
29+
static_assert(std::is_assignable<std::variant<char>, int>::value == VariantAllowsNarrowingConversions, "");
30+
31+
static_assert(std::is_assignable<std::variant<std::string, float>, int>::value == VariantAllowsNarrowingConversions, "");
32+
static_assert(std::is_assignable<std::variant<std::string, double>, int>::value == VariantAllowsNarrowingConversions, "");
33+
static_assert(!std::is_assignable<std::variant<std::string, bool>, int>::value, "");
34+
35+
static_assert(!std::is_assignable<std::variant<int, bool>, decltype("meow")>::value, "");
36+
static_assert(!std::is_assignable<std::variant<int, const bool>, decltype("meow")>::value, "");
37+
static_assert(!std::is_assignable<std::variant<int, const volatile bool>, decltype("meow")>::value, "");
38+
39+
static_assert(!std::is_assignable<std::variant<bool>, std::true_type>::value, "");
40+
static_assert(!std::is_assignable<std::variant<bool>, std::unique_ptr<char> >::value, "");
41+
static_assert(!std::is_assignable<std::variant<bool>, decltype(nullptr)>::value, "");
42+
43+
}

libcxx/test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ void test_T_ctor_sfinae() {
6969
}
7070
{
7171
using V = std::variant<std::string, float>;
72-
static_assert(!std::is_constructible<V, int>::value,
72+
static_assert(std::is_constructible<V, int>::value == VariantAllowsNarrowingConversions,
7373
"no matching constructor");
7474
}
7575
{
@@ -127,11 +127,13 @@ void test_T_ctor_basic() {
127127
static_assert(v.index() == 1, "");
128128
static_assert(std::get<1>(v) == 42, "");
129129
}
130+
#ifndef TEST_VARIANT_ALLOWS_NARROWING_CONVERSIONS
130131
{
131132
constexpr std::variant<unsigned, long> v(42);
132133
static_assert(v.index() == 1, "");
133134
static_assert(std::get<1>(v) == 42, "");
134135
}
136+
#endif
135137
{
136138
std::variant<std::string, bool const> v = "foo";
137139
assert(v.index() == 0);

libcxx/test/std/utilities/variant/variant.variant/variant.ctor/conv.fail.cpp

Lines changed: 0 additions & 39 deletions
This file was deleted.
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
// -*- C++ -*-
2+
//===----------------------------------------------------------------------===//
3+
//
4+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
5+
// See https://llvm.org/LICENSE.txt for license information.
6+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
7+
//
8+
//===----------------------------------------------------------------------===//
9+
10+
// UNSUPPORTED: c++98, c++03, c++11, c++14
11+
12+
// <variant>
13+
14+
// template <class ...Types> class variant;
15+
16+
// template <class T> constexpr variant(T&&) noexcept(see below);
17+
18+
#include <variant>
19+
#include <string>
20+
#include <memory>
21+
22+
#include "variant_test_helpers.hpp"
23+
24+
int main(int, char**)
25+
{
26+
static_assert(!std::is_constructible<std::variant<int, int>, int>::value, "");
27+
static_assert(!std::is_constructible<std::variant<long, long long>, int>::value, "");
28+
static_assert(std::is_constructible<std::variant<char>, int>::value == VariantAllowsNarrowingConversions, "");
29+
30+
static_assert(std::is_constructible<std::variant<std::string, float>, int>::value == VariantAllowsNarrowingConversions, "");
31+
static_assert(std::is_constructible<std::variant<std::string, double>, int>::value == VariantAllowsNarrowingConversions, "");
32+
static_assert(!std::is_constructible<std::variant<std::string, bool>, int>::value, "");
33+
34+
static_assert(!std::is_constructible<std::variant<int, bool>, decltype("meow")>::value, "");
35+
static_assert(!std::is_constructible<std::variant<int, const bool>, decltype("meow")>::value, "");
36+
static_assert(!std::is_constructible<std::variant<int, const volatile bool>, decltype("meow")>::value, "");
37+
38+
static_assert(!std::is_constructible<std::variant<bool>, std::true_type>::value, "");
39+
static_assert(!std::is_constructible<std::variant<bool>, std::unique_ptr<char> >::value, "");
40+
static_assert(!std::is_constructible<std::variant<bool>, decltype(nullptr)>::value, "");
41+
42+
}

libcxx/test/support/variant_test_helpers.hpp

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,15 @@
2121

2222
// FIXME: Currently the variant<T&> tests are disabled using this macro.
2323
#define TEST_VARIANT_HAS_NO_REFERENCES
24+
#ifdef _LIBCPP_ENABLE_NARROWING_CONVERSIONS_IN_VARIANT
25+
# define TEST_VARIANT_ALLOWS_NARROWING_CONVERSIONS
26+
#endif
27+
28+
#ifdef TEST_VARIANT_ALLOWS_NARROWING_CONVERSIONS
29+
constexpr bool VariantAllowsNarrowingConversions = true;
30+
#else
31+
constexpr bool VariantAllowsNarrowingConversions = false;
32+
#endif
2433

2534
#ifndef TEST_HAS_NO_EXCEPTIONS
2635
struct CopyThrows {

0 commit comments

Comments
 (0)