When moving a 'optional<T&>', don't move the remote value #93

jiixyj · 2024-12-31T11:28:53Z

Moving optional<T&> into optional<T> should probably not result in a move of the remote value. Because of reference collapsing, the constraints of the move constructor/assignment operator of optional<T> already check for a copy of the remote value, not a move.

This makes the code match the constraints by ~~moving the move before the dereference, since for optional<T&>, *std::move(rhs) always results in a lvalue reference, not a rvalue reference.~~ introducing new constructor/assignment overloads for optional<T&>.

edit: fixes #92
edit: updated for new approach

coveralls · 2024-12-31T11:32:36Z

coverage: 93.22% (+0.1%) from 93.091%
when pulling e5fab32 on jiixyj:shallow-value-category-on-move
into 5460182 on bemanproject:main.

neatudarius · 2025-01-02T10:46:02Z

src/beman/optional26/tests/optional.t.cpp

+// Moving an `optional<T&>` should not move the remote value.
+// Check this by deleting the rvalue overloads of some (unrealistic) types.
+TEST(OptionalTest, MoveRef) {
+    struct copyable_but_not_movable {


same obersation - we have a header file with test classes in src/beman/optional26/tests/test_types.hpp

Thanks for the pointer! I'll update the PRs accordingly.

steve-downey · 2025-01-02T12:31:28Z

include/beman/optional26/optional.hpp

@@ -488,7 +488,7 @@ inline constexpr optional<T>::optional(optional<U>&& rhs)
    requires detail::enable_from_other<T, U, U&&> && std::is_convertible_v<U&&, T>
 {
    if (rhs.has_value()) {


https://eel.is/c++draft/optional#ctor-34 means this needs a paper.

Yep, and there is https://eel.is/c++draft/optional#assign-25 as well.

Although in a world where only optional<T> exists I believe std::move(*rhs) and *std::move(rhs) do the same thing. So implementers could have used the as-if rule this whole time. Only for optional<T&> there might be a semantic difference. Maybe P2988 is a good vehicle for this?

steve-downey · 2025-01-02T12:34:24Z

src/beman/optional26/tests/optional.t.cpp

@@ -868,3 +868,42 @@ TEST(OptionalTest, HashTest) {
        EXPECT_EQ(h1, h2);
    }
 }
+


tl::optional and boost::optional both fail to compile this, so it's not a problem unique to beman::optional.

With beman::optional, this also fails to compile. The title of the PR lead me to think this was doing the wrong thing successfully. Do you have a case where moving an optional<T&> incorrectly moves the referred to value?

With beman::optional, this also fails to compile. The title of the PR lead me to think this was doing the wrong thing successfully. Do you have a case where moving an optional<T&> incorrectly moves the referred to value?

Yes, I have a more realistic example at the issue: #92

steve-downey · 2025-01-02T19:40:05Z

If it's that a optional<Foo> will steal the referred to of optional<Foo&> in the converting assignment, it might be safer to specialize the assignment operator? If U is a reference it should always copy assign regardless of the value category of the optional.

…

On Thu, Jan 2, 2025, 13:15 Jan Kokemüller ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/beman/optional26/tests/optional.t.cpp <#93 (comment)> : > @@ -868,3 +868,42 @@ TEST(OptionalTest, HashTest) { EXPECT_EQ(h1, h2); } } + +// Moving an `optional<T&>` should not move the remote value. +// Check this by deleting the rvalue overloads of some (unrealistic) types. +TEST(OptionalTest, MoveRef) { + struct copyable_but_not_movable { Thanks for the pointer! I'll update the PRs accordingly. — Reply to this email directly, view it on GitHub <#93 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAVNZ5QDQEC4CPCEHMW4B6T2IV63HAVCNFSM6AAAAABUNLRXROVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMRYGAYTSMZUGU> . You are receiving this because your review was requested.Message ID: ***@***.***>

jiixyj · 2025-01-02T21:51:00Z

If it's that a optional will steal the referred to of optional<Foo&> in the converting assignment, it might be safer to specialize the assignment operator? If U is a reference it should always copy assign regardless of the value category of the optional.

This sounds sensible to me! It occured to me that the constraints are slightly wrong even for the optional(const optional& rhs) converting constructor when called with a optional<U&>, so having some overloads for optional<T&> would be more correct. Did you have something like the following in mind?

    template <class U>
    constexpr explicit(!std::is_convertible_v<U, T>) optional(const optional<U>& rhs)
        requires(!std::is_reference_v<U> && detail::enable_from_other<T, U, const U&> && std::is_convertible_v<const U&, T>);
    template <class U>
    constexpr explicit(!std::is_convertible_v<U, T>) optional(const optional<U>& rhs)
        requires(!std::is_reference_v<U> && detail::enable_from_other<T, U, const U&> && !std::is_convertible_v<const U&, T>);

    template <class U>
    constexpr explicit(!std::is_convertible_v<U, T>) optional(optional<U>&& rhs)
        requires(!std::is_reference_v<U> && detail::enable_from_other<T, U, U &&> && std::is_convertible_v<U &&, T>);
    template <class U>
    constexpr explicit(!std::is_convertible_v<U, T>) optional(optional<U>&& rhs)
        requires(!std::is_reference_v<U> && detail::enable_from_other<T, U, U &&> && !std::is_convertible_v<U &&, T>);

    template <class U>
    constexpr explicit(!std::is_convertible_v<U, T>) optional(const optional<U&>& rhs)
        requires(detail::enable_from_other<T, U, U&> && std::is_convertible_v<U&, T>);
    template <class U>
    constexpr explicit(!std::is_convertible_v<U, T>) optional(const optional<U&>& rhs)
        requires(detail::enable_from_other<T, U, U&> && !std::is_convertible_v<U&, T>);

...this adds !std::is_reference_v constraints to the optional overloads, and the constraints for optional<U&> check for constructibility from U& (without const, since const is shallow).

steve-downey · 2025-01-02T22:04:34Z

Or using an `if constexpr` to change the body of the affected assignment operators, as is_reference doesn't affect participation in overload, just what the 'Effects' are for the assignment, where for an optional<U&> it should always treat the U as an lvalue, which I think is what *std::move(rhs) ended up doing, but by more complicated machinery. The cv qualifiers and value category of the optional are shallow, so not relevant, so there might be only one overload necessary on the reference side? I think having the explicit !std::is_reference_v and std::is_reference_v also avoids subsumption problems in the compiler in checking the requires clauses, which is partly why my inclination would be to if constexpr in the body.

…

On Thu, Jan 2, 2025 at 4:51 PM Jan Kokemüller ***@***.***> wrote: If it's that a optional will steal the referred to of optional<Foo&> in the converting assignment, it might be safer to specialize the assignment operator? If U is a reference it should always copy assign regardless of the value category of the optional. This sounds sensible to me! It occured to me that the constraints are slightly wrong even for the optional(const optional& rhs) converting constructor when called with a optional<U&>, so having some overloads for optional<T&> would be more correct. Did you have something like the following in mind? template <class U> constexpr explicit(!std::is_convertible_v<U, T>) optional(const optional& rhs) requires(!std::is_reference_v && detail::enable_from_other<T, U, const U&> && std::is_convertible_v<const U&, T>); template <class U> constexpr explicit(!std::is_convertible_v<U, T>) optional(const optional& rhs) requires(!std::is_reference_v && detail::enable_from_other<T, U, const U&> && !std::is_convertible_v<const U&, T>); template <class U> constexpr explicit(!std::is_convertible_v<U, T>) optional(optional&& rhs) requires(!std::is_reference_v && detail::enable_from_other<T, U, U &&> && std::is_convertible_v); template <class U> constexpr explicit(!std::is_convertible_v<U, T>) optional(optional&& rhs) requires(!std::is_reference_v && detail::enable_from_other<T, U, U &&> && !std::is_convertible_v); template <class U> constexpr explicit(!std::is_convertible_v<U, T>) optional(const optional<U&>& rhs) requires(detail::enable_from_other<T, U, U&> && std::is_convertible_v<U&, T>); template <class U> constexpr explicit(!std::is_convertible_v<U, T>) optional(const optional<U&>& rhs) requires(detail::enable_from_other<T, U, U&> && !std::is_convertible_v<U&, T>); ...this adds !std::is_reference_v constraints to the optional overloads, and the constraints for optional<U&> check for constructibility from U& (without const, since const is shallow). — Reply to this email directly, view it on GitHub <#93 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAVNZ5SJXNFKIG6DUXKVUI32IWYFVAVCNFSM6AAAAABUNLRXROVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNRYGQZDIOBZHA> . You are receiving this because your review was requested.Message ID: ***@***.***>

…'optional<U&>'

jiixyj · 2025-01-03T15:20:51Z

I've implemented the approach with the separated overloads for optional<U&>. And added some more exhaustive tests to really make sure the desired operations on the U& are called. It is now nicely symmetrical compared to the monadic operations that also always operate on the U& regardless of the value category of the optional<U&>.

Or using an if constexpr to change the body of the affected assignment operators

Can you elaborate? I'm not sure how this would look like, as the constraints are different for all the overloads, especially the detail::enable_from_other. For example it would be bad if the detail::enable_from_other checked constructibility from const U&, but then inside the body one would construct from U&.

steve-downey · 2025-01-03T15:39:41Z

That sounds like it might not work, then, although I think that's also a problem for the initial approach because the result of the optional deref operation can be different? What you've described sounds like a much better approach, though. It does mean I'll be busy in the next week to update P2988.

…

On Fri, Jan 3, 2025, 10:21 Jan Kokemüller ***@***.***> wrote: I've implemented the approach with the separated overloads for optional<U&>. And added some more exhaustive tests to really make sure the desired operations on the U& are called. It is now nicely symmetrical compared to the monadic operations that also always operate on the U& regardless of the value category of the optional<U&>. Or using an if constexpr to change the body of the affected assignment operators Can you elaborate? I'm not sure how this would look like, as the constraints are different for all the overloads, especially the detail::enable_from_other. For example it would be bad if the detail::enable_from_other checked constructibility from const U&, but then inside the body one would construct from U&. — Reply to this email directly, view it on GitHub <#93 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAVNZ5WFSWYJNGD6UNLQDNL2I2TGVAVCNFSM6AAAAABUNLRXROVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNRZGM4DSNZQHA> . You are receiving this because your review was requested.Message ID: ***@***.***>

steve-downey · 2025-01-04T18:40:03Z

A simple set of failing test cases

TEST(OptionalTest, TestStealingFromReference) {
    beman::optional26::tests::copyable_move_detector                               m;
    beman::optional26::optional<beman::optional26::tests::copyable_move_detector&> om{m};
    beman::optional26::optional<beman::optional26::tests::copyable_move_detector>  o{std::move(om)};
    EXPECT_FALSE(m.been_moved);
}

TEST(OptionalTest, TestAssignStealingFromReference) {
    beman::optional26::tests::copyable_move_detector                               m;
    beman::optional26::optional<beman::optional26::tests::copyable_move_detector&> om{m};
    beman::optional26::optional<beman::optional26::tests::copyable_move_detector>  o;
    o = std::move(om);
    EXPECT_FALSE(m.been_moved);
}

where copyable_move_detector is

struct copyable_move_detector {
    constexpr copyable_move_detector()                            = default;
    constexpr copyable_move_detector(const copyable_move_detector&) = default;
    constexpr copyable_move_detector(copyable_move_detector&& rhs) { rhs.been_moved = true; }
    constexpr copyable_move_detector& operator=(const copyable_move_detector&) = default;
    constexpr copyable_move_detector& operator=(copyable_move_detector&& rhs) { rhs.been_moved = true; return *this;}

    bool been_moved = false;
};

(not modifying move_detector for fear of affecting something depending on it not being copyable at the moment.)

That is - these tests fail on main and pass on this branch.

steve-downey · 2025-01-06T09:47:35Z

Can you merge main back into this branch to resolve conflicts? Otherwise I'll do so from the command line and push.

…y-on-move

jiixyj · 2025-01-06T16:15:01Z

Can you merge main back into this branch to resolve conflicts? Otherwise I'll do so from the command line and push.

Sure thing, I've just merged the main branch back.

When moving a 'optional<T&>', don't move the remote value

3632ace

jiixyj requested review from camio, neatudarius and steve-downey as code owners December 31, 2024 11:28

jiixyj mentioned this pull request Dec 31, 2024

What should moving from optional<T&> into optional<T> do? #92

Closed

neatudarius reviewed Jan 2, 2025

View reviewed changes

steve-downey reviewed Jan 2, 2025

View reviewed changes

jiixyj added 2 commits January 3, 2025 16:02

Add separate constructors and assignment operators from 'optional<T&>'

c891a22

Fix middle argument to 'enable_from_other' for construct/assign from …

ab3eedf

…'optional<U&>'

steve-downey approved these changes Jan 6, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into shallow-value-categor…

e5fab32

…y-on-move

steve-downey merged commit bb60b34 into bemanproject:main Jan 6, 2025
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When moving a 'optional<T&>', don't move the remote value #93

When moving a 'optional<T&>', don't move the remote value #93

jiixyj commented Dec 31, 2024 •

edited

Loading

coveralls commented Dec 31, 2024 •

edited

Loading

neatudarius Jan 2, 2025

jiixyj Jan 2, 2025

steve-downey Jan 2, 2025

jiixyj Jan 2, 2025

steve-downey Jan 2, 2025

jiixyj Jan 2, 2025

steve-downey commented Jan 2, 2025 via email

jiixyj commented Jan 2, 2025

steve-downey commented Jan 2, 2025 via email

jiixyj commented Jan 3, 2025

steve-downey commented Jan 3, 2025 via email

steve-downey commented Jan 4, 2025 •

edited

Loading

steve-downey commented Jan 6, 2025

jiixyj commented Jan 6, 2025

When moving a 'optional<T&>', don't move the remote value #93

When moving a 'optional<T&>', don't move the remote value #93

Conversation

jiixyj commented Dec 31, 2024 • edited Loading

coveralls commented Dec 31, 2024 • edited Loading

neatudarius Jan 2, 2025

Choose a reason for hiding this comment

jiixyj Jan 2, 2025

Choose a reason for hiding this comment

steve-downey Jan 2, 2025

Choose a reason for hiding this comment

jiixyj Jan 2, 2025

Choose a reason for hiding this comment

steve-downey Jan 2, 2025

Choose a reason for hiding this comment

jiixyj Jan 2, 2025

Choose a reason for hiding this comment

steve-downey commented Jan 2, 2025 via email

jiixyj commented Jan 2, 2025

steve-downey commented Jan 2, 2025 via email

jiixyj commented Jan 3, 2025

steve-downey commented Jan 3, 2025 via email

steve-downey commented Jan 4, 2025 • edited Loading

steve-downey commented Jan 6, 2025

jiixyj commented Jan 6, 2025

jiixyj commented Dec 31, 2024 •

edited

Loading

coveralls commented Dec 31, 2024 •

edited

Loading

steve-downey commented Jan 4, 2025 •

edited

Loading