mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Park <mp...@apache.org>
Subject Re: Review Request 68813: Added support for `Option<T&>` / `Option<const T&>`.
Date Thu, 04 Oct 2018 15:25:58 GMT


> On Sept. 23, 2018, 4:31 a.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();
>     }
>     ```
> 
> Benjamin Bannier wrote:
>     > 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.

I think one thing to consider is what we want `Option` to be.
Is it a temporary stand-in for `std::optional` until it can be replaced?
or are we unsatisfied with the `std::optional` design and we want `Option`
to actually be a different thing? IIRC, even for `Option<T>`, assignment
semantics are deviate from `std::optional`.

---

As for the standard, we __need__ to answer questions around what equality
and assignment means for references. We don't have to here...

---

Regarding the lifetime concerns,

```
hashmap<U, V> fun();
Option<V&> value = fun().get(key);
```

This is unsafe regardless of whether there's an `Option` there or not.
Even if we were to have used `const V&` with `at`, that `hashmap`
falls on its face and we have a dangling reference.

So it can't be on `Option` to solve that problem.

---

I've already mentioned this to bmahler, but as far as return types are
concerned, I would personally just opt for `T*` rather than `Option<T&>`.

```
// Now, I'm not assuming the key is present, so naturally,
// I get an optional reference instead of the reference:
T* value = hashmap.get(key);

if (value) {
  value->foo();
}
```

Considering only this use case (not worrying about stuff like comparison
semantics since it's not our primary use case at least at the moment),
the only change is `value.isSome()` vs `value` as the `if` condition.

---

To me the benefit of `Option<T&>` is as a function parameter, since taking
a `T*` there doesn't allow passing temporaries (which is safe in a function
call context) and passing a `T` to `const Option<T>&` incurs a copy.

If we want this use case of passing a temporary, we cannot delete
the rvalue reference ctor.


- Michael


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68813/#review208927
-----------------------------------------------------------


On Sept. 22, 2018, 6:09 p.m., Benjamin Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68813/
> -----------------------------------------------------------
> 
> (Updated Sept. 22, 2018, 6:09 p.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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message