> On June 16, 2017, 1:25 p.m., Benjamin Bannier wrote: > > I do not agree with this change. > > > > An `Owned` cannot be copied semantically. If we pass an `Owned` at all, it > > should be by value, not by reference, so potential copies happen on > > interface boundaries and we do not need to perform potentially unneeded > > copies inside the consuming functions. See e.g., > > https://herbsutter.com/2013/06/05/gotw-91-solution-smart-pointer-parameters/ > > for a more detailed write-up. > > > > I believe the existing code is still broken since it performs copies of an > > `Owned` (which is currently allowed syntactically, > > https://issues.apache.org/jira/browse/MESOS-5122). IMHO the correct fix > > would be to keep the existing signatures, but to `std::move` when passing > > the `authenticator` value on, both when used as function argument and when > > ultimatly stored in `AuthenticatorManagerProcess::authenticators_`. This > > would both prevent copies and express ownership semantics correctly. > > Neil Conway wrote: > In principle, I agree with you 100%. `Owned` should have `unique_ptr` > semantics (or we should just remove `Owned` and use `unique_ptr`), and a > function that takes over the ownership of an `Owned` pointer should use > `std::move`. > > But until `Owned` actually enforces `unique_ptr` semantics and disallows > copies, do we actually want to encourage people using `std::move`? We use > `std::move` relatively rarely, and in the places where we use it, we often > get it wrong :) In contrast, we use `const Owned<T>&` parameters reasonably > often right now... > > At least in this case, we'd need to add quite a few `std::move`s to > account for all the call-sites. I guess for now I'll just give up on this, > and we can try to clean it up properly as part of fixing `Owned`.
> But until Owned actually enforces unique_ptr semantics and disallows copies, > do we actually want to encourage people using std::move? I believe we should and I point out incorrect usage if I come across it. Using `Owned` incorrectly (e.g., copying them) can introduce blatant bugs which are not very apparent from looking at variable declarations alone. Semantically correct use of `Owned` from the start will also ease the transition to a true owning smart pointer (which we will need to tackle eventually). - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60139/#review178098 ----------------------------------------------------------- On June 16, 2017, 1:16 a.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60139/ > ----------------------------------------------------------- > > (Updated June 16, 2017, 1:16 a.m.) > > > Review request for mesos and Alexander Rojas. > > > Repository: mesos > > > Description > ------- > > Spotted via clang-tidy. > > > Diffs > ----- > > 3rdparty/libprocess/src/authenticator_manager.hpp > 0dc8fd24b411d649bcc62208bde5784cac4ea997 > 3rdparty/libprocess/src/authenticator_manager.cpp > 5cbed53e7085f227d90679e1b56ad803d9b74a47 > > > Diff: https://reviews.apache.org/r/60139/diff/1/ > > > Testing > ------- > > `make check` > > > Thanks, > > Neil Conway > >
