mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 68813: Added support for `Option<T&>` / `Option<const T&>`.
Date Mon, 24 Sep 2018 23:46:21 GMT


> On Sept. 23, 2018, 11: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.

> 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


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


On Sept. 23, 2018, 1: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, 1: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
> 
>


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