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