From 1d93a02cc471534f84406aec33fb2bf4bddf1779 Mon Sep 17 00:00:00 2001 From: Giuseppe D'Angelo Date: Thu, 19 Jun 2025 22:54:35 +0200 Subject: [PATCH 1/3] optional::transform() const &&: move the value into the function If *this is an rvalue reference, we're required to move the value contained into the function passed to transform(). Do that. --- include/beman/optional/optional.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/beman/optional/optional.hpp b/include/beman/optional/optional.hpp index f7b8abb9..9abf89d8 100644 --- a/include/beman/optional/optional.hpp +++ b/include/beman/optional/optional.hpp @@ -818,7 +818,7 @@ constexpr auto optional::transform(F&& f) const&& { static_assert(!std::is_same_v); static_assert(!std::is_same_v); static_assert(std::is_object_v || std::is_reference_v); /// References now allowed - return (has_value()) ? optional{std::invoke(std::forward(f), value_)} : optional{}; + return (has_value()) ? optional{std::invoke(std::forward(f), std::move(value_))} : optional{}; } /// Calls `f` if the optional is empty From 75532d44b838b1580ab25ce4212fb4bd1a8536ba Mon Sep 17 00:00:00 2001 From: Giuseppe D'Angelo Date: Fri, 20 Jun 2025 10:59:13 +0200 Subject: [PATCH 2/3] optional::transform: ensure that it works with non-movable payloads This is supposed to work and it's explicitly spelled out in the standard text. If the function called by transform returns a non-movable prvalue, we cannot rely on optional(T&&) constructor for this, because that will not simply materialize it into the payload of the returned optional. Instead, add a new private constructor for optional with a private tag. Inside that constructor, we invoke the function and use it to initialize the payload; this will not require any moves due to guaranteed copy elision. In transform(), we call that constructor, passing the function and the value stored in *this to it. A similar change is needed for the optional. Since we need to call this private optional's constructor from a generic optional::transform, we need to grant friendship to all possible optional specializations. --- include/beman/optional/optional.hpp | 31 ++++++++++++++++--- tests/beman/optional/optional_monadic.t.cpp | 20 ++++++++++++ .../beman/optional/optional_ref_monadic.t.cpp | 21 +++++++++++++ 3 files changed, 67 insertions(+), 5 deletions(-) diff --git a/include/beman/optional/optional.hpp b/include/beman/optional/optional.hpp index 9abf89d8..59a53cde 100644 --- a/include/beman/optional/optional.hpp +++ b/include/beman/optional/optional.hpp @@ -49,6 +49,12 @@ template concept optional_ge_rel = requires(const T& t, const U& u) { { t >= u } -> std::convertible_to; }; + +struct from_function_t { + explicit from_function_t() = default; +}; + +inline constexpr from_function_t from_function{}; } // namespace detail struct in_place_t { @@ -403,6 +409,13 @@ class optional { std::destroy_at(std::addressof(value_)); engaged_ = false; } + + template + friend class optional; + + template + constexpr optional(detail::from_function_t, F&& f, Arg&& arg) + : value_(std::invoke(std::forward(f), std::forward(arg))), engaged_(true) {} }; class bad_optional_access : public std::exception { @@ -785,7 +798,7 @@ constexpr auto optional::transform(F&& f) & { static_assert(!std::is_same_v); static_assert(!std::is_same_v); static_assert(std::is_object_v || std::is_reference_v); /// References now allowed - return (has_value()) ? optional{std::invoke(std::forward(f), value_)} : optional{}; + return (has_value()) ? optional{detail::from_function, std::forward(f), value_} : optional{}; } template @@ -796,7 +809,7 @@ constexpr auto optional::transform(F&& f) && { static_assert(!std::is_same_v); static_assert(!std::is_same_v); static_assert(std::is_object_v || std::is_reference_v); /// References now allowed - return (has_value()) ? optional{std::invoke(std::forward(f), std::move(value_))} : optional{}; + return (has_value()) ? optional{detail::from_function, std::forward(f), std::move(value_)} : optional{}; } template @@ -807,7 +820,7 @@ constexpr auto optional::transform(F&& f) const& { static_assert(!std::is_same_v); static_assert(!std::is_same_v); static_assert(std::is_object_v || std::is_reference_v); /// References now allowed - return (has_value()) ? optional{std::invoke(std::forward(f), value_)} : optional{}; + return (has_value()) ? optional{detail::from_function, std::forward(f), value_} : optional{}; } template @@ -818,7 +831,7 @@ constexpr auto optional::transform(F&& f) const&& { static_assert(!std::is_same_v); static_assert(!std::is_same_v); static_assert(std::is_object_v || std::is_reference_v); /// References now allowed - return (has_value()) ? optional{std::invoke(std::forward(f), std::move(value_))} : optional{}; + return (has_value()) ? optional{detail::from_function, std::forward(f), std::move(value_)} : optional{}; } /// Calls `f` if the optional is empty @@ -1207,6 +1220,14 @@ class optional { T& r(std::forward(u)); value_ = std::addressof(r); } + + template + friend class optional; + + template + constexpr optional(detail::from_function_t, F&& f, Arg&& arg) { + convert_ref_init_val(std::invoke(std::forward(f), std::forward(arg))); + } }; // \rSec3[optionalref.ctor]{Constructors} @@ -1356,7 +1377,7 @@ constexpr optional> optional::transform(F&& f) c static_assert((std::is_object_v && !std::is_array_v) || std::is_lvalue_reference_v, "Result must be an non-array object or an lvalue reference"); if (has_value()) { - return optional{std::invoke(std::forward(f), *value_)}; + return optional{detail::from_function, std::forward(f), *value_}; } else { return optional{}; } diff --git a/tests/beman/optional/optional_monadic.t.cpp b/tests/beman/optional/optional_monadic.t.cpp index 2e4afa97..6d7d8358 100644 --- a/tests/beman/optional/optional_monadic.t.cpp +++ b/tests/beman/optional/optional_monadic.t.cpp @@ -3,6 +3,8 @@ #include +#include + #include constexpr int get_int(int) { return 42; } @@ -81,6 +83,24 @@ TEST(OptionalMonadicTest, Transform) { beman::optional::optional o38r = o38.transform([](int& i) -> const int& { return i; }); EXPECT_TRUE(o38r); EXPECT_TRUE(*o38r == 42); + + // transform and return a non-movable class + using immovable = beman::optional::tests::immovable; + beman::optional::optional o39 = 42; + auto o39r = o39.transform([](int) { return immovable(); }); + EXPECT_TRUE(o39r); + + beman::optional::optional o40 = 42; + auto o40r = std::move(o40).transform([](int) { return immovable(); }); + EXPECT_TRUE(o40r); + + const beman::optional::optional o41 = 42; + auto o41r = o41.transform([](int) { return immovable(); }); + EXPECT_TRUE(o41r); + + const beman::optional::optional o42 = 42; + auto o42r = std::move(o42).transform([](int) { return immovable(); }); + EXPECT_TRUE(o42r); } TEST(OptionalMonadicTest, TransformConstexpr) { diff --git a/tests/beman/optional/optional_ref_monadic.t.cpp b/tests/beman/optional/optional_ref_monadic.t.cpp index 143bc9d0..43473621 100644 --- a/tests/beman/optional/optional_ref_monadic.t.cpp +++ b/tests/beman/optional/optional_ref_monadic.t.cpp @@ -3,6 +3,8 @@ #include +#include + #include namespace { @@ -89,6 +91,25 @@ TEST(OptionalRefMonadicTest, Transform) { auto o38r = o38.transform([](int& i) -> const int& { return i; }); EXPECT_TRUE(o38r); EXPECT_TRUE(*o38r == 42); + + // transform and return a non-movable class + using immovable = beman::optional::tests::immovable; + + beman::optional::optional o39 = fortytwo; + auto o39r = o39.transform([](int&) { return immovable(); }); + EXPECT_TRUE(o39r); + + beman::optional::optional o40 = fortytwo; + auto o40r = std::move(o40).transform([](int&) { return immovable(); }); + EXPECT_TRUE(o40r); + + const beman::optional::optional o41 = fortytwo; + auto o41r = o41.transform([](int&) { return immovable(); }); + EXPECT_TRUE(o41r); + + const beman::optional::optional o42 = fortytwo; + auto o42r = std::move(o42).transform([](int&) { return immovable(); }); + EXPECT_TRUE(o42r); } TEST(OptionalRefMonadicTest, TransformConstexpr) { From 6b5137ff7f3230ff075c939753436da083acc093 Mon Sep 17 00:00:00 2001 From: Giuseppe D'Angelo Date: Fri, 20 Jun 2025 14:43:24 +0200 Subject: [PATCH 3/3] optional::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 we were doing exactly that (`return *this` or `move(*this)`). For optional we were instead doing `return *value_`, where `value_` is the pointer used in the implementation. That ends up creating an optional 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::or_else, the implementation is isomorphic to the second form, not to the first one: ``` T *obj = new T; optional ref = *obj; delete obj; assert(ref); // OK optional ref2 = ref.or_else(/* ... */); // UB; does *obj internally ``` We can avoid this UB by avoiding the dereference into optional::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. --- include/beman/optional/optional.hpp | 2 +- tests/beman/optional/optional_monadic.t.cpp | 25 +++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/include/beman/optional/optional.hpp b/include/beman/optional/optional.hpp index 59a53cde..5561266a 100644 --- a/include/beman/optional/optional.hpp +++ b/include/beman/optional/optional.hpp @@ -1389,7 +1389,7 @@ constexpr optional optional::or_else(F&& f) const { using U = std::invoke_result_t; static_assert(std::is_same_v, optional>, "Result must be an optional"); if (has_value()) { - return *value_; + return *this; } else { return std::forward(f)(); } diff --git a/tests/beman/optional/optional_monadic.t.cpp b/tests/beman/optional/optional_monadic.t.cpp index 6d7d8358..40c604a1 100644 --- a/tests/beman/optional/optional_monadic.t.cpp +++ b/tests/beman/optional/optional_monadic.t.cpp @@ -343,3 +343,28 @@ TEST(OptionalMonadicTest, Constexpr_or_else) { constexpr auto test4 = *(std::move(o2).or_else([] { return beman::optional::make_optional(13); })) == 13; EXPECT_TRUE(test4); } + +constexpr bool optional_or_else_do_not_dereference_impl() { + // create a dangling optional + int* ptr = new int(42); + beman::optional::optional iref = *ptr; + delete ptr; + + if (!iref) + return false; + + const auto or_else_fun = []() { return beman::optional::optional(); }; + + // This must not dereference the pointer inside `iref` + beman::optional::optional iref2 = iref.or_else(or_else_fun); + if (!iref2) + return false; + + return true; +} + +static_assert(optional_or_else_do_not_dereference_impl()); + +TEST(OptionalMonadicTest, optional_or_else_do_not_derefence) { + EXPECT_TRUE(optional_or_else_do_not_dereference_impl()); +}