Add support for Maybe<T&>
Categories
(Core :: MFBT, task)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox76 | --- | fixed |
People
(Reporter: sg, Assigned: sg)
Details
Attachments
(2 files)
Having support for Maybe<T&> would enable to replace raw pointers, which may be nullptr, by a safer variant.
+1,000,000! (Coincidentally I was thinking about that yesterday, but all points to you for opening this bug.)
Note that "There are no std::optional references; a program is ill-formed if it instantiates an optional with a reference type. Alternatively, an optional of a std::reference_wrapper of type T may be used to hold a reference." (from https://en.cppreference.com/w/cpp/utility/optional .)
I'm not sure why this was rejected (maybe because references cannot be changed, but you could re-assign an optional<T&>?), it could be useful to see if there's anything really bad.
In any case, I'm personally tentatively for this improvement. 👍
Just in case: Another alternative could to optimize Maybe<NotNull<T*>> so it's only storing a pointer, with null checks instead of a bool mIsSome. But it's more verbose and ugly.
|
Assignee | |
Comment 2•5 years ago
|
||
I am aware that std::optional<T&> was rejected, but I am neither aware of the reasoning. boost::optional<T&> is supported, however: https://www.boost.org/doc/libs/1_67_0/libs/optional/doc/html/boost_optional/tutorial/optional_references.html
Indeed, Maybe<NotNull<T*>> might functionally be an alternative, but, IIUC, it would expose a pointer again, which is somewhat in conflict what I would like to achieve in usage.
If Maybe<T&> is considered to be too different from other Maybe<T>, it could be implemented under a different name, e.g. MaybeRef<T>.
|
|
Assignee | |
Comment 3•5 years ago
|
||
|
||
Updated•5 years ago
|
|
||
Comment 4•5 years ago
|
||
optional<T&> was apparently rejected on the grounds that nobody could agree on what operator= did:
https://www.fluentcpp.com/2018/10/05/pros-cons-optional-references/
Simon's patch sidesteps that issue by not having operator=. At least one interweb person believes that optional<T&> shouldn't be a thing anyway:
https://foonathan.net/2018/07/optional-reference/
Although that doesn't cover members, which I suppose is really the desired use-case here?
|
|
Assignee | |
Comment 5•5 years ago
|
||
I wasn't thinking about members at the moment, but indeed about function parameters and getters.
About that, foonathan.net says:
The only situations where it might be feasible to have a std::optional<T&> are function parameters and getters where we want to avoid a copy.
but then waives it away by
This is not such a compelling use-case.
It's not just to avoid a copy: OTOH, the type might not be copyable at all, but more compellingly, obviously getting a value and a reference are semantically different. I do think that's a compelling use case to avoid to use raw pointers. A Maybe<T&> makes it more explicit (and comes with automatic assertions) that this is not owning and optional.
As I wrote above, I can understand that Maybe<T> for non-reference T and Maybe<T&> might seem not that related, and this might better be added under a different name, but I think it would be good to have it.
Gerald, what use cases did you have in mind?
My own use case would have been to return/pass a maybe-reference to a member of a class that has an in-session/out-0f-session state. At the moment I return/pass pointers: return mInSession ? &mStuff : nullptr;.
I admit I hadn't thought deeply about Maybe<T&>. But the more I read and think about it, the scarier and more confusing it gets! So I'm less enthused now, sorry.
E.g., I can imagine reading future code like Maybe<int&> x = ...; Maybe<int&> y = ...; x = y; and wondering "is that a value copy, or changing what x refers to?" -- Probably the latter, otherwise the code should probably be *x = *y;, right? But the point is that writers&reviewers may too easily get it wrong, or spend too much time thinking about each instance.
Oh, now I see you didn't define copy&assignment, so maybe it's not as scary.
Great, now I'm confused whether I'm still for or against it. 😅 I'm happier with it now, but I'll need to let it stew for a while... (But please don't wait for me.)
|
|
||
Comment 8•5 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Comment 9•5 years ago
|
||
|
|
||
Updated•5 years ago
|
|
||
Comment 10•5 years ago
|
||
|
|
||
Comment 11•5 years ago
|
||
| bugherder | ||
Description
•