mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 60139: Avoided needless copy of `Owned<T>` in libprocess.
Date Fri, 16 Jun 2017 19:21:16 GMT


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


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