Miscellanea fixes for optional #128

dangelog · 2025-06-24T15:12:07Z

These commits implement some miscellanea fixes for beman::optional. I'm basing them on P2988R12.

I think the dangling case is a real defect in the specification and deserves a discussion in LWG. The others are just implementation fixes.

If *this is an rvalue reference, we're required to move the value contained into the function passed to transform(). Do that.

coveralls · 2025-06-24T15:15:45Z

Pull Request Test Coverage Report for Build 16011929600

Details

11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 16007560103:	0.0%
Covered Lines:	311
Relevant Lines:	311

💛 - Coveralls

neatudarius

LGTM, but also wait for @steve-downey review

steve-downey

Sorry not to take a look at this sooner.

I want to make sure I understand the changes before merging, particularly the dangling issue.

Do you have an NB comment drafted to ask for a fix to the spec if we need it? If so, I can copy/paste to my list to go to INCITS.

Also it looks like a number of my questions are answered in the individual commit messages, so I'll be re-reading those.

(marking "request changes" so I have time to answer my own questions. I'm not expecting @dangelog to do more work)

include/beman/optional/optional.hpp

steve-downey · 2025-07-01T19:01:58Z

tests/beman/optional/optional_monadic.t.cpp

    EXPECT_TRUE(o38r);
    EXPECT_TRUE(*o38r == 42);
+
+    // transform and return a non-movable class


These are the tests I missed for transform and the reason for the from_function tag?

Do existing implementations get this right? Do we have a spec bug, or is silence enough to require it to work?

Well, the spec says "[Note 1: There is no requirement that U is movable ([dcl.init.general]).
— end note]" in https://eel.is/c++draft/optional#monadic-note-1

Implementations support such non-movable payloads: https://gcc.godbolt.org/z/Wh6q7EGsj

tests/beman/optional/optional_ref_monadic.t.cpp

dangelog · 2025-07-01T21:40:27Z

Hi! :-)

Sorry not to take a look at this sooner.

I want to make sure I understand the changes before merging, particularly the dangling issue.

Sure thing, I think I've explained in the associated commit message; basically I'm afraid that doing *value_ when value_ is a dangling dangling is instantly UB per https://eel.is/c++draft/expr.unary.op#1.sentence-4 .

(Since then *value_ yields a T& and or_else returns a optional<T&>, this will go through the optional<T&>::optional(U&&) constructor to create the return value.)

Instead we could just copy *this -- go through optional<T&>'s copy constructor. Formally, that copies the value_ pointer, and copying a dangling pointer is still implementation-defined (https://eel.is/c++draft/basic.compound#4), but that still sounds a better approach.

At least that's my understanding of the matter...

Do you have an NB comment drafted to ask for a fix to the spec if we need it? If so, I can copy/paste to my list to go to INCITS.

Is the wording for optional<T&> from the paper already merged? I think I can raise it as a LWG issue, together with the other couple of minor edits. Or would you prefer to file this as NB comment?

Also it looks like a number of my questions are answered in the individual commit messages, so I'll be re-reading those.

(marking "request changes" so I have time to answer my own questions. I'm not expecting @dangelog to do more work)

I'm happy to help and contribute a little!

steve-downey · 2025-07-01T21:49:06Z

Now that I've realized the commits on the PR branch have excellent comments, this is all good for me to review. Thank you for the work!

…

On Tue, Jul 1, 2025, 17:30 dangelog ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In include/beman/optional/optional.hpp <#128 (comment)> : > @@ -1368,7 +1389,7 @@ constexpr optional<T&> optional<T&>::or_else(F&& f) const { using U = std::invoke_result_t<F>; static_assert(std::is_same_v<std::remove_cvref_t<U>, optional>, "Result must be an optional"); if (has_value()) { - return *value_; + return *this; I'm sorry, I really dislike (to put it mildly) GitHub review workflow. The rationale is documented here in the commit message: a1ba572 <a1ba572> Does it make sense to you? Would you like me to open individual PRs instead? I pushed a branch because the commits depend on each other... — Reply to this email directly, view it on GitHub <#128 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAVNZ5XQS5VEWL34YNTP5OD3GL4WLAVCNFSM6AAAAACAAUVD2SVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDSNZXGAYTMMRZGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

steve-downey · 2025-07-01T22:07:03Z

On Tue, Jul 1, 2025, 17:40 dangelog ***@***.***> wrote: *dangelog* left a comment (bemanproject/optional#128) <#128 (comment)> Hi! :-) Sorry not to take a look at this sooner. I want to make sure I understand the changes before merging, particularly the dangling issue. Sure thing, I think I've explained in the associated commit message; basically I'm afraid that doing *value_ when value_ is a dangling dangling is instantly UB per https://eel.is/c++draft/expr.unary.op#1.sentence-4 . (Since then *value_ yields a T& and or_else returns a optional<T&>, this will go through the optional<T&>::optional(U&&) constructor to create the return value.) Instead we could just copy *this -- go through optional<T&>'s copy constructor. Formally, that copies the value_ pointer, and copying a dangling pointer is still implementation-defined ( https://eel.is/c++draft/basic.compound#4), but that still sounds a better approach.

Ah! Of course. I was, because I ran into the optional<T> and <T&> bug, thinking of the operator* producing a T&, not just producing an optional<U>& to copy construct. I agree that copying the value of the pointer, while not safe, is safe-er in the face of an already dangling pointer.

At least that's my understanding of the matter... Do you have an NB comment drafted to ask for a fix to the spec if we need it? If so, I can copy/paste to my list to go to INCITS. Is the wording for optional<T&> from the paper already merged? I think I can raise it as a LWG issue, together with the other couple of minor edits. Or would you prefer to file this as NB comment?

If it hasn't been merged, it will be soon. A Library issue is always good, but an NB comment saying apply the fix for an issue makes sure it's done at Kona.

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<T&>. Since we need to call this private optional<U>'s constructor from a generic optional<T>::transform, we need to grant friendship to all possible optional specializations.

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.

JeffGarland · 2025-07-02T02:35:18Z

At least that's my understanding of the matter... Do you have an NB comment drafted to ask for a fix to the spec if we need it? If so, I can copy/paste to my list to go to INCITS. Is the wording for optional<T&> from the paper already merged? I think I can raise it as a LWG issue, together with the other couple of minor edits. Or would you prefer to file this as NB comment?
If it hasn't been merged, it will be soon. A Library issue is always good, but an NB comment saying apply the fix for an issue makes sure it's done at Kona.

Please file an LWG issue here - this is always the best way: https://cplusplus.github.io/LWG/lwg-active.html#submit_issue

It will get attention without an NB comment (likely as P1 since it hasn't shipped), but if someone wants the NB comment can become fix LWG whatever-the-number. Since it's likely you'll have a resolution LWG can handle it promptly via list review without even going to NB route or even Kona. I expect we will do significant issue processing prior to Kona.

Because I'm now satisfied I understand the changes here.

steve-downey · 2025-07-08T08:16:54Z

include/beman/optional/optional.hpp

        std::destroy_at(std::addressof(value_));
        engaged_ = false;
    }
+


Reviewing for creating LWG issues:
I believe this, which is necessary for https://eel.is/c++draft/optional.monadic#note-1, is already covered by the current wording.

steve-downey · 2025-07-08T08:26:45Z

include/beman/optional/optional.hpp

    static_assert(std::is_same_v<std::remove_cvref_t<U>, optional>, "Result must be an optional");
    if (has_value()) {
-        return *value_;
+        return *this;


This is the LWG issue fodder. We wish to change from constructing a new optional over the deferenced val, to using the converting constructor for optional from optional, in order to avoid the deref of a possibly dangling pointer and instead copy the value of the pointer. This may not be safe on all platforms, but is safer and defined on many.

optional<T>::transform() const &&: move the value into the function

1d93a02

If *this is an rvalue reference, we're required to move the value contained into the function passed to transform(). Do that.

dangelog requested review from camio, neatudarius and steve-downey as code owners June 24, 2025 15:12

dangelog requested review from a team, RaduNichita and wusatosi June 24, 2025 15:12

neatudarius approved these changes Jul 1, 2025

View reviewed changes

steve-downey previously requested changes Jul 1, 2025

View reviewed changes

dangelog added 2 commits July 2, 2025 00:57

dangelog force-pushed the P2988R12_misc_fixes branch from a1ba572 to 6b5137f Compare July 1, 2025 22:57

steve-downey approved these changes Jul 8, 2025

View reviewed changes

steve-downey merged commit 0195cf3 into bemanproject:main Jul 8, 2025
11 checks passed

Miscellanea fixes for optional #128

Miscellanea fixes for optional #128

Uh oh!

Conversation

dangelog commented Jun 24, 2025

Uh oh!

coveralls commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 16011929600

Details

💛 - Coveralls

Uh oh!

neatudarius left a comment

Choose a reason for hiding this comment

Uh oh!

steve-downey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

steve-downey Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

dangelog Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dangelog commented Jul 1, 2025

Uh oh!

steve-downey commented Jul 1, 2025 via email

Uh oh!

steve-downey commented Jul 1, 2025 via email

Uh oh!

JeffGarland commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steve-downey Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

steve-downey Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Jun 24, 2025 •

edited

Loading

JeffGarland commented Jul 2, 2025 •

edited

Loading