Skip to content

Commit 6b5137f

Browse files
committed
optional<T&>::or_else: copy *this instead of creating a new optional
If or_else is called on an engaged optional, we're supposed to return a copy (or a move) of the `*this` object; otherwise we invoke the argument of or_else, and return whatever optional that returns. For optional<T> we were doing exactly that (`return *this` or `move(*this)`). For optional<T&> we were instead doing `return *value_`, where `value_` is the pointer used in the implementation. That ends up creating an optional<T&> through its "value constructor". The problem is that the two forms are not equivalent in corner cases; consider this code: ``` T *obj = new T; T &ref = *obj; delete obj; // obj now dangles T &ref2 = ref; // OK ``` The last line is OK even if `ref` does not refer to an object any more. This code is instead not OK: ``` T *obj = new T; T &ref = *obj; delete obj; T &ref2 = *obj; // UB, https://eel.is/c++draft/expr.unary.op#1 ``` If we use optional<T&>::or_else, the implementation is isomorphic to the second form, not to the first one: ``` T *obj = new T; optional<T &> ref = *obj; delete obj; assert(ref); // OK optional<T &> ref2 = ref.or_else(/* ... */); // UB; does *obj internally ``` We can avoid this UB by avoiding the dereference into optional<T&>::or_else, and returning a copy of *this instead. The semantics are otherwise the same, but we avoid tripping into UB. I'm adding a test which however is inconclusive because compilers do not detect the above UB during constant evaluation, although they're required to do so. That's likely a bug.
1 parent 75532d4 commit 6b5137f

File tree

2 files changed

+26
-1
lines changed

2 files changed

+26
-1
lines changed

include/beman/optional/optional.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1389,7 +1389,7 @@ constexpr optional<T&> optional<T&>::or_else(F&& f) const {
13891389
using U = std::invoke_result_t<F>;
13901390
static_assert(std::is_same_v<std::remove_cvref_t<U>, optional>, "Result must be an optional");
13911391
if (has_value()) {
1392-
return *value_;
1392+
return *this;
13931393
} else {
13941394
return std::forward<F>(f)();
13951395
}

tests/beman/optional/optional_monadic.t.cpp

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -343,3 +343,28 @@ TEST(OptionalMonadicTest, Constexpr_or_else) {
343343
constexpr auto test4 = *(std::move(o2).or_else([] { return beman::optional::make_optional(13); })) == 13;
344344
EXPECT_TRUE(test4);
345345
}
346+
347+
constexpr bool optional_or_else_do_not_dereference_impl() {
348+
// create a dangling optional<T&>
349+
int* ptr = new int(42);
350+
beman::optional::optional<int&> iref = *ptr;
351+
delete ptr;
352+
353+
if (!iref)
354+
return false;
355+
356+
const auto or_else_fun = []() { return beman::optional::optional<int&>(); };
357+
358+
// This must not dereference the pointer inside `iref`
359+
beman::optional::optional<int&> iref2 = iref.or_else(or_else_fun);
360+
if (!iref2)
361+
return false;
362+
363+
return true;
364+
}
365+
366+
static_assert(optional_or_else_do_not_dereference_impl());
367+
368+
TEST(OptionalMonadicTest, optional_or_else_do_not_derefence) {
369+
EXPECT_TRUE(optional_or_else_do_not_dereference_impl());
370+
}

0 commit comments

Comments
 (0)