> On Sept. 23, 2018, 1:31 p.m., Benjamin Bannier wrote:
> > I am not convinced we should add this. The alternative of using e.g., an
> > `Option<T*>` or `Option<T const*>` seems to not only produce correct
> > behavior (even when wrapping a ptr to `const`), but also caution users
> > enough that noting here protects against dangling references or performs
> > any reference lifetime extension. While this seems redundant in the case of
> > `Option` where one could just return a `nullptr` for `None` values, such a
> > pattern would translate seemlessly to e.g., `Try` or `Result`, and the
> > behavior of empty case could be solved by documentation wherever we would
> > return such a type. It would also avoid unusual semantics around assignment
> > or comparision, and would e.g., continue to support hashing (the type
> > proposed here does not support `hash`).
> >
> > I'd suggest to drop this patch and instead use wrappers around pointers if
> > we really want to provide such behavior in lieu of e.g., `contains` checks
> > and returning naked values.
>
> Benjamin Mahler wrote:
> > performs any reference lifetime extension
>
> Can't we just delete the rvalue reference constructor to prevent the user
> from trying to extend lifetime? This seems to be what boost did?
>
>
> https://www.boost.org/doc/libs/1_68_0/libs/optional/doc/html/boost_optional/tutorial/optional_references.html
>
> > It would also avoid unusual semantics around assignment or comparision
>
> Isn't this patch already avoiding these by disabling them?
>
> > I'd suggest to drop this patch and instead use wrappers around pointers
> if we really want to provide such behavior in lieu of e.g., contains checks
> and returning naked values.
>
> Are you suggesting code like this?
>
> ```
> Option<T*> value = hashmap.get(key);
>
> if (value.isSome()) {
> (*value)->foo();
> }
> ```
>
> This doesn't feel quite a clean as:
>
> ```
> T& value = hashmap.at(key);
>
> // use value.
>
> // Now, I'm not assuming the key is present, so naturally,
> // I get an optional reference instead of the reference:
> Option<T&> value = hashmap.get(key);
>
> if (value.isSome()) {
> value->foo();
> }
> ```
> Can't we just delete the rvalue reference constructor to prevent the user
> from trying to extend lifetime?
Consider e.g.,
```
hashmap<U, V> fun();
Option<V&> value = fun().get(key); // Allowed if `hashmap::get` not forbidden
for `&&`.
```
To make such code safe any function returning an `Option<T&>` would need to be
disabled for rvalue `this` values explicitly; there seems there is nothing we
can do in `Option<T&>`'s definition to make this safe in general. I am not sure
we would be able to prevent bad code slipping in across the board in normal
human on human reviews.
>> It would also avoid unusual semantics around assignment or comparision
> Isn't this patch already avoiding these by disabling them?
Yes, it does. What I meant is that the way these new `Option<T&>` values can be
handled is suprisingly different from "normal" `Option<T>` values; they e.g.,
cannot be compared against each other or values of the wrapped type, or cannot
be put into `set`s or `hashmap`s (the latter could likely be fixed). We loose
some of the power that `Option<T>` brings because due to its value semantics. I
am unsure there is a lot benefit left at that point.
> Are you suggesting code like this?
> [...]
I was suggesting to use the existing
```
// Pretty safe `value`; some dangerous patterns likely recognizable.
if (hashmap.contains(key)) {
T& value = hashmap.at(key);
value->foo();
}
```
or if we really wanted to expose reference semantics with all the extra rope it
brings
```
// Completely unsafe `value`; type leaves no illusion on safety,
// users and reviewers hopefully on alert.
Option<T*> value = hashmap.get(key);
if (value.isSome()) {
value.get()->foo();
}
```
My _I am not convinced_ was sincere as I am really not sure whether these are
strong enough arguments or just me being change-averse. Maybe @mpark has some
insights he can share from the trenches of `variant<T&>` becoming forbidden.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68813/#review208927
-----------------------------------------------------------
On Sept. 23, 2018, 3:09 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68813/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2018, 3:09 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benno Evers, and Michael Park.
>
>
> Bugs: MESOS-9252
> https://issues.apache.org/jira/browse/MESOS-9252
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This adds support for options of references. While this is still
> under debate for `std::optional`, there are some use cases in
> stout that can benefit from this:
>
> // None if the value is not found, otherwise a reference
> // to the value.
> Option<T&> t = hashmap.get("key");
>
> Assignment and equality are deleted in order to avoid confusion
> around which of the 2 possible behaviors they provide (e.g. are
> the references being compared? or are the objects being referred
> to being compared?)
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/option.hpp
> 8feed012a55fed6eab89c883958324f3345e46e9
>
>
> Diff: https://reviews.apache.org/r/68813/diff/1/
>
>
> Testing
> -------
>
> Test added in subsequent patch.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
