> On June 16, 2017, 11:25 a.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.
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`. - Neil ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60139/#review178098 ----------------------------------------------------------- On June 15, 2017, 11:16 p.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60139/ > ----------------------------------------------------------- > > (Updated June 15, 2017, 11:16 p.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 > >
