1620568 - Add support for Maybe<T&>

Status: RESOLVED FIXED

(Core :: MFBT, task)

Description

[Simon Giesecke [:sg] [he/him]]

Having support for Maybe<T&> would enable to replace raw pointers, which may be nullptr, by a safer variant.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

+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.

Comment 2

[Simon Giesecke [:sg] [he/him]]

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>.

Comment 3

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1620568 - Add support for Maybe<T&>. r=froydnj

Comment 4

[Nathan Froyd [:froydnj]]

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?

Comment 5

[Simon Giesecke [:sg] [he/him]]

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?

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57f9a3f2c406
Add support for Maybe<T&>. r=froydnj

Comment 8

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/57f9a3f2c406

Comment 9

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1620568 - Disable equality comparison operators for Maybe<T&>. r=froydnj

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87516ee10624
Disable equality comparison operators for Maybe<T&>. r=froydnj

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/87516ee10624