Skip to content

Commit 933c4cf

Browse files
snarkmastermeta-codesync[bot]
snarkmaster
authored and
meta-codesync[bot]
committed
Make value_only_result copyable
Summary: Context in D92859871, which made `result` copyable. Reviewed By: janondrusek Differential Revision: D92867241 fbshipit-source-id: 75478f4a93f9b8558d33bbc1af551f123ac090f2
1 parent 5f3c74b commit 933c4cf

File tree

5 files changed

+191
-35
lines changed

5 files changed

+191
-35
lines changed

‎folly/result/BUCK‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -226,5 +226,8 @@ fb_dirsync_cpp_library(
226226
],
227227
use_raw_headers = True,
228228
xplat_impl = folly_xplat_cxx_library,
229-
exported_deps = ["//folly/result:coro"],
229+
exported_deps = [
230+
":result",
231+
"//folly/result:coro",
232+
],
230233
)

‎folly/result/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,7 @@ folly_add_library(
206206
value_only_result_coro.h
207207
EXPORTED_DEPS
208208
folly_result_coro
209+
folly_result_result
209210
)
210211

211212
add_subdirectory(detail)

‎folly/result/test/BUCK‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -342,7 +342,6 @@ fb_dirsync_cpp_unittest(
342342
name = "value_only_result_test",
343343
srcs = ["value_only_result_test.cpp"],
344344
deps = [
345-
"//folly/portability:gtest",
346345
"//folly/result:gtest_helpers",
347346
"//folly/result:value_only_result",
348347
],

‎folly/result/test/value_only_result_test.cpp‎

Lines changed: 136 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -25,15 +25,76 @@
2525

2626
namespace folly::test {
2727

28+
// Test self-copy-assignment without triggering `-Wself-assign-overloaded`.
29+
template <typename T>
30+
void selfCopy(T& x) {
31+
auto* p = &x;
32+
x = *p;
33+
}
34+
35+
// value_only_result<V>: copyable when V is, movable
36+
static_assert(std::is_copy_constructible_v<value_only_result<int>>);
37+
static_assert(std::is_copy_assignable_v<value_only_result<int>>);
38+
static_assert(std::is_move_constructible_v<value_only_result<int>>);
39+
static_assert(std::is_move_assignable_v<value_only_result<int>>);
40+
41+
// value_only_result<V> with move-only V: not copyable, movable
42+
static_assert(
43+
!std::is_copy_constructible_v<value_only_result<std::unique_ptr<int>>>);
44+
static_assert(
45+
!std::is_copy_assignable_v<value_only_result<std::unique_ptr<int>>>);
46+
static_assert(
47+
std::is_move_constructible_v<value_only_result<std::unique_ptr<int>>>);
48+
static_assert(
49+
std::is_move_assignable_v<value_only_result<std::unique_ptr<int>>>);
50+
51+
// value_only_result<V&>: copyable from mutable only (deep-const).
52+
// `is_copy_constructible_v` tests `const&`, so it's correctly FALSE here.
53+
static_assert(!std::is_copy_constructible_v<value_only_result<int&>>);
54+
static_assert(
55+
std::is_constructible_v<value_only_result<int&>, value_only_result<int&>&>);
56+
static_assert(!std::is_constructible_v<
57+
value_only_result<int&>,
58+
const value_only_result<int&>&>);
59+
static_assert(!std::is_constructible_v<
60+
value_only_result<int&>,
61+
const value_only_result<int&>&&>);
62+
static_assert(!std::is_copy_assignable_v<value_only_result<int&>>);
63+
static_assert(
64+
std::is_assignable_v<value_only_result<int&>&, value_only_result<int&>&>);
65+
static_assert(!std::is_assignable_v<
66+
value_only_result<int&>&,
67+
const value_only_result<int&>&>);
68+
static_assert(!std::is_assignable_v<
69+
value_only_result<int&>&,
70+
const value_only_result<int&>&&>);
71+
static_assert(std::is_move_constructible_v<value_only_result<int&>>);
72+
static_assert(std::is_move_assignable_v<value_only_result<int&>>);
73+
74+
// value_only_result<const V&>: fully copyable
75+
static_assert(std::is_copy_constructible_v<value_only_result<const int&>>);
76+
static_assert(std::is_copy_assignable_v<value_only_result<const int&>>);
77+
static_assert(std::is_move_constructible_v<value_only_result<const int&>>);
78+
static_assert(std::is_move_assignable_v<value_only_result<const int&>>);
79+
80+
// value_only_result<V&&>: move-only
81+
static_assert(!std::is_copy_constructible_v<value_only_result<int&&>>);
82+
static_assert(!std::is_copy_assignable_v<value_only_result<int&&>>);
83+
static_assert(std::is_move_constructible_v<value_only_result<int&&>>);
84+
static_assert(std::is_move_assignable_v<value_only_result<int&&>>);
85+
86+
// value_only_result<void>: copyable & movable
87+
static_assert(std::is_copy_constructible_v<value_only_result<>>);
88+
static_assert(std::is_copy_assignable_v<value_only_result<>>);
89+
static_assert(std::is_move_constructible_v<value_only_result<>>);
90+
static_assert(std::is_move_assignable_v<value_only_result<>>);
91+
2892
// Fully tests `void`-specific behaviors. Loosely covers common features from
2993
// `value_only_result_crtp` -- the subsequent non-`void` tests cover them more.
3094
TEST(ValueOnlyResult, ofVoid) {
3195
// Cover the handful of things specific to the `void` specialization, plus
3296
// copyability & movability.
3397
{
34-
static_assert(!std::is_copy_constructible_v<value_only_result<>>);
35-
static_assert(!std::is_copy_assignable_v<value_only_result<>>);
36-
3798
value_only_result<> r;
3899
static_assert(std::is_same_v<decltype(r), value_only_result<void>>);
39100
static_assert(std::is_void_v<decltype(r.value_or_throw())>);
@@ -46,8 +107,16 @@ TEST(ValueOnlyResult, ofVoid) {
46107
value_only_result<> r3;
47108
r3 = std::move(r2); // move-assign
48109

49-
r3.copy().value_or_throw();
50-
r3.copy().value_only();
110+
copy(r3).value_or_throw();
111+
copy(r3).value_only();
112+
113+
{ // copy-assign: value <- value
114+
value_only_result<> rc;
115+
rc = r3;
116+
EXPECT_TRUE(rc.has_value());
117+
}
118+
selfCopy(r3);
119+
EXPECT_TRUE(r3.has_value());
51120

52121
EXPECT_TRUE(result<void>(r3).has_value()); // explicit conversion to result
53122
}
@@ -89,7 +158,7 @@ TEST(ValueOnlyResult, refCopiable) {
89158
std::is_same_v<
90159
value_only_result<std::unique_ptr<int>&>,
91160
decltype(mIntPtrRef1)>);
92-
auto mIntPtrRef2 = mIntPtrRef1.copy();
161+
auto mIntPtrRef2 = value_only_result{mIntPtrRef1};
93162
*(mIntPtrRef2.value_or_throw()) += 1;
94163
*(mIntPtrRef2.value_only()) += 10;
95164
EXPECT_EQ(1348, *mIntPtrRef1.value_or_throw());
@@ -99,9 +168,22 @@ TEST(ValueOnlyResult, refCopiable) {
99168

100169
TEST(ValueOnlyResult, copyMethod) {
101170
value_only_result<int> r{1337};
102-
auto rToo = r.copy();
171+
auto rToo{r};
103172
EXPECT_EQ(r.value_or_throw(), rToo.value_only());
104173
EXPECT_TRUE(r == rToo);
174+
175+
// Copy assignment
176+
value_only_result<int> rc{0};
177+
rc = rToo;
178+
EXPECT_EQ(1337, rc.value_or_throw());
179+
180+
// Non-trivially-copyable
181+
value_only_result<std::string> rs{std::string("hi")};
182+
auto rsToo{rs};
183+
EXPECT_EQ("hi", rsToo.value_or_throw());
184+
185+
selfCopy(r);
186+
EXPECT_EQ(1337, r.value_or_throw());
105187
}
106188

107189
RESULT_CO_TEST(ValueOnlyResult, ofLvalueReferenceWrapper) {
@@ -145,27 +227,60 @@ RESULT_CO_TEST(Result, forbidUnsafeCopyOfResultRef) {
145227
value_only_result rc = std::cref(n);
146228
static_assert(std::is_same_v<value_only_result<const int&>, decltype(rc)>);
147229
{ // Safe copies of ref -- `rc` has `const` inside, cannot be discarded
148-
value_only_result rc2 = rc.copy();
230+
value_only_result rc2{rc};
149231
EXPECT_EQ(42, (co_await or_unwind(rc2)));
150-
value_only_result rc3 = std::as_const(rc).copy();
232+
value_only_result rc3{std::as_const(rc)};
151233
EXPECT_EQ(42, (co_await or_unwind(rc3)));
152234
}
153-
static_assert(requires { rc.copy(); });
154-
static_assert(requires { std::as_const(rc).copy(); });
235+
static_assert(
236+
std::is_constructible_v<
237+
value_only_result<const int&>,
238+
value_only_result<const int&>&>);
239+
static_assert(
240+
std::is_constructible_v<
241+
value_only_result<const int&>,
242+
const value_only_result<const int&>&>);
243+
// Copy-assign const ref result
244+
{
245+
int n2 = 99;
246+
value_only_result<const int&> rcA = std::cref(n);
247+
value_only_result<const int&> rcB = std::cref(n2);
248+
rcA = rcB;
249+
EXPECT_EQ(99, rcA.value_or_throw());
250+
EXPECT_EQ(&n2, &rcA.value_or_throw()); // rebinding check
251+
}
155252

156253
value_only_result<int&> r = std::ref(n);
157254
{ // Safe copy of ref -- `r` has no `const` to discard
158-
value_only_result r2 = r.copy();
255+
value_only_result r2{r};
159256
EXPECT_EQ(42, (co_await or_unwind(r2)));
160257
}
161258
// Unsafe: copying `const value_only_result<int&>` would discard the `const`
162-
// result r3 = std::as_const(r).copy();
163-
// The next assert shows the above `.copy()` is SFINAE-deleted.
164-
//
165-
// NB: This `requires` won't compile without using a dependent type.
166-
static_assert(![](const auto& r2) { return requires { r2.copy(); }; }(r));
259+
static_assert(
260+
!std::is_constructible_v<
261+
value_only_result<int&>,
262+
const value_only_result<int&>&>);
167263
// Copy-from-mutable still works
168-
static_assert([](auto& r2) { return requires { r2.copy(); }; }(r));
264+
static_assert(
265+
std::is_constructible_v<
266+
value_only_result<int&>,
267+
value_only_result<int&>&>);
268+
// Copy-assign mutable ref result (rebinding)
269+
{
270+
int a = 10, b = 20;
271+
value_only_result<int&> ra = std::ref(a);
272+
value_only_result<int&> rb = std::ref(b);
273+
ra = rb;
274+
EXPECT_EQ(20, ra.value_or_throw());
275+
EXPECT_EQ(&b, &ra.value_or_throw()); // rebinding check
276+
}
277+
// Cannot copy-assign from const source for mutable ref
278+
static_assert(
279+
!std::is_assignable_v<
280+
value_only_result<int&>&,
281+
const value_only_result<int&>&>);
282+
selfCopy(r);
283+
EXPECT_EQ(&n, &r.value_or_throw());
169284
}
170285

171286
// Check `co_await` / `.value_or_throw()` / `.value_only()` for various ways of
@@ -251,7 +366,7 @@ RESULT_CO_TEST(ValueOnlyResult, fromRefWrapperAndRefAccess) {
251366
T t2 = std::make_unique<int>(567);
252367
{
253368
value_only_result<T&> rLref = std::ref(t1);
254-
EXPECT_EQ(321, *(co_await or_unwind(rLref.copy())));
369+
EXPECT_EQ(321, *(co_await or_unwind(value_only_result{rLref})));
255370
EXPECT_EQ(321, *rLref.value_or_throw());
256371
EXPECT_EQ(321, *rLref.value_only());
257372
*(co_await or_unwind(rLref)) += 1;
@@ -262,7 +377,7 @@ RESULT_CO_TEST(ValueOnlyResult, fromRefWrapperAndRefAccess) {
262377

263378
{
264379
value_only_result<const T&> rCref = std::cref(t1);
265-
EXPECT_EQ(567, *(co_await or_unwind(rCref.copy())));
380+
EXPECT_EQ(567, *(co_await or_unwind(copy(rCref))));
266381
EXPECT_EQ(567, *rCref.value_or_throw());
267382
EXPECT_EQ(567, *rCref.value_only());
268383
// can change the int, not the unique_ptr --
@@ -272,7 +387,7 @@ RESULT_CO_TEST(ValueOnlyResult, fromRefWrapperAndRefAccess) {
272387
EXPECT_TRUE(t2 == nullptr); // was moved out above
273388
t2 = std::make_unique<int>(42);
274389
rCref = std::cref(t2); // assignment uses the implict ctor
275-
EXPECT_EQ(42, *(co_await or_unwind(rCref.copy())));
390+
EXPECT_EQ(42, *(co_await or_unwind(copy(rCref))));
276391
}
277392

278393
{

‎folly/result/value_only_result.h‎

Lines changed: 50 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -94,10 +94,6 @@ class value_only_result_crtp {
9494
template <typename>
9595
friend class folly::value_only_result; // The simple conversion uses `value_`
9696

97-
struct private_copy_t {};
98-
value_only_result_crtp(private_copy_t, const Derived& that)
99-
: value_(that.value_) {}
100-
10197
template <typename V>
10298
value_only_result_crtp(std::in_place_t, V&& v)
10399
: value_(static_cast<V&&>(v)) {}
@@ -111,26 +107,31 @@ class value_only_result_crtp {
111107
value_only_result_crtp(value_only_result_crtp&&) = default;
112108
value_only_result_crtp& operator=(value_only_result_crtp&&) = default;
113109

114-
/// Explicit `.copy()` method instead of a standard copy constructor.
110+
[[deprecated(
111+
"value_only_result<T> is copyable;"
112+
" use folly::copy() for explicit copies")]]
115113
Derived copy()
116114
requires(
117115
!std::is_rvalue_reference_v<T> &&
118116
(std::is_void_v<T> || std::is_copy_constructible_v<T>))
119117
{
120-
return Derived{private_copy_t{}, static_cast<const Derived&>(*this)};
118+
return static_cast<Derived&>(*this);
121119
}
122120
// The `const`-safety constraints are explained on `result::copy`.
121+
[[deprecated(
122+
"value_only_result<T> is copyable;"
123+
" use folly::copy() for explicit copies")]]
123124
Derived copy() const
124125
requires(
125126
(!std::is_reference_v<T> ||
126127
std::is_const_v<std::remove_reference_t<T>>) &&
127128
(std::is_void_v<T> || std::is_copy_constructible_v<T>))
128129
{
129-
return Derived{private_copy_t{}, static_cast<const Derived&>(*this)};
130+
return static_cast<const Derived&>(*this);
130131
}
131-
// IMPORTANT: Read the `result` copy ctor comment before touching.
132-
value_only_result_crtp(const value_only_result_crtp&) = delete;
133-
value_only_result_crtp& operator=(const value_only_result_crtp&) = delete;
132+
// Leaf classes provide copy ctors with deep-const constraints for refs.
133+
value_only_result_crtp(const value_only_result_crtp&) = default;
134+
value_only_result_crtp& operator=(const value_only_result_crtp&) = default;
134135

135136
// NB: This is currently missing the counterpart to "Fallible copy/move
136137
// conversion" from `result.h`. That's because it seems very unlikely that
@@ -156,13 +157,47 @@ class [[nodiscard]]
156157
using base = detail::value_only_result_crtp<value_only_result<T>, T>;
157158
// For `T` non-`void`, we store either `T` or a ref wrapper.
158159
using ref_wrapped_t = typename base::storage_type;
160+
// `true` for `value_only_result<V&>` where `V` is non-const. These use
161+
// deep-const copy semantics: only copyable from a mutable source.
162+
static constexpr bool is_mutable_lref_v = std::is_lvalue_reference_v<T> &&
163+
!std::is_const_v<std::remove_reference_t<T>>;
159164

160165
public:
161166
using detail::value_only_result_crtp<value_only_result<T>, T>::
162167
value_only_result_crtp;
163168

164169
/// Future: add default-constructibility iff `result` gets it.
165170

171+
/// Movable, copyable. See `result.h` for copy semantics.
172+
value_only_result(value_only_result&&) = default;
173+
value_only_result& operator=(value_only_result&&) = default;
174+
175+
// Copy from `const value_only_result&`: value types and const-lref types.
176+
value_only_result(const value_only_result&)
177+
requires(!std::is_rvalue_reference_v<T> && !is_mutable_lref_v &&
178+
std::is_copy_constructible_v<ref_wrapped_t>)
179+
= default;
180+
// Copy from mutable `value_only_result&`: non-const lvalue refs.
181+
value_only_result(value_only_result& that)
182+
requires(is_mutable_lref_v)
183+
: base{that} {}
184+
185+
// Copy-assign from `const value_only_result&`.
186+
value_only_result& operator=(const value_only_result&)
187+
requires(!std::is_rvalue_reference_v<T> && !is_mutable_lref_v &&
188+
std::is_copy_constructible_v<ref_wrapped_t> &&
189+
std::is_copy_assignable_v<ref_wrapped_t>)
190+
= default;
191+
// Copy-assign from mutable `value_only_result&`.
192+
value_only_result& operator=(value_only_result& that)
193+
requires(
194+
is_mutable_lref_v &&
195+
std::is_assignable_v<ref_wrapped_t&, ref_wrapped_t>)
196+
{
197+
this->value_ = that.value_;
198+
return *this;
199+
}
200+
166201
/// Copy- & move-conversion from a reference wrapper.
167202
/* implicit */ value_only_result(ref_wrapped_t t) noexcept
168203
requires std::is_reference_v<T>
@@ -193,8 +228,6 @@ class [[nodiscard]]
193228
sizeof(T) <= hardware_constructive_interference_size)
194229
: base{std::in_place, t} {}
195230

196-
/// No copy assignment, just like `result`.
197-
198231
/// Simple copy/move conversion.
199232
///
200233
/// Convert `value_only_result<U>` to `value_only_result<T>` if:
@@ -340,6 +373,11 @@ class [[nodiscard]]
340373

341374
value_only_result() : base(std::in_place, unit) {}
342375

376+
value_only_result(value_only_result&&) = default;
377+
value_only_result& operator=(value_only_result&&) = default;
378+
value_only_result(const value_only_result&) = default;
379+
value_only_result& operator=(const value_only_result&) = default;
380+
343381
void value_or_throw() const noexcept {}
344382
void value_only() const noexcept {}
345383

0 commit comments

Comments
 (0)