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 11:25:25 GMT

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



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.

- Benjamin Bannier


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