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 46824: Fully qualified addresses of Flag members in add calls.
Date Tue, 05 Jul 2016 08:34:44 GMT


> On May 11, 2016, 11:27 p.m., Vinod Kone wrote:
> > src/local/main.cpp, lines 52-53
> > <https://reviews.apache.org/r/46824/diff/3/?file=1379588#file1379588line52>
> >
> >     why the need for "::" prefix here?

There appears to be no need for it; remove here and elsewhere.


> On May 11, 2016, 11:27 p.m., Vinod Kone wrote:
> > src/master/main.cpp, lines 229-245
> > <https://reviews.apache.org/r/46824/diff/3/?file=1379589#file1379589line229>
> >
> >     where is this code moved from and why?

This seems to have slipped in when rebasing. Thanks for the careful review.


> On May 11, 2016, 11:27 p.m., Vinod Kone wrote:
> > src/master/main.cpp, line 131
> > <https://reviews.apache.org/r/46824/diff/3/?file=1379589#file1379589line131>
> >
> >     If `add` should be only called by derived classes, should `add` method be protected
instead of public?

I am not sure `add` should only be called from derived classes, and since we force callers
to pass in a member variable pointer these usages appear fine to me. Since most (all?) calls
to `add` are from classes derived from `FlagsBase` making it `protected` instead of `public`
would be possible, but I wouldn't be sure why that would be needed. It might also potentially
make some valid use cases impossible.


- Benjamin


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


On July 5, 2016, 10:33 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46824/
> -----------------------------------------------------------
> 
> (Updated July 5, 2016, 10:33 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Bugs: MESOS-3335
>     https://issues.apache.org/jira/browse/MESOS-3335
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> While right now we can technically `add` variables to `Flags` classes
> which are not members, the in order to have correct copy semantics for
> `Flags` only member variables should be used.
> 
> Here we changed all instances to a full pointer-to-member syntax in
> the current code.
> 
> 
> Diffs
> -----
> 
>   src/cli/execute.cpp 18c2e3449bf5e50bea0bbd0a92efa20fc8b9032b 
>   src/cli/resolve.cpp ac6be05e64c5e66703081068d992ebb0d6ca6c70 
>   src/docker/executor.hpp 7b63d784d6b8685912598b77fb38cf6e70646ae3 
>   src/examples/balloon_framework.cpp 5613ff0c61e2d2f84684a206debc97dcb8b2c0d3 
>   src/examples/dynamic_reservation_framework.cpp 850bb2a5b243dd5d5a8b6476570b4f943fde6185

>   src/examples/load_generator_framework.cpp 5402e31b89b7ead1dc8fdc9065980b5b1c0d380c

>   src/examples/long_lived_framework.cpp 7c57eb5e4a34208504475013690ae8e3cae74155 
>   src/examples/no_executor_framework.cpp 57425726aa5ca27c9579b0d8ecc0bb9eb9b26852 
>   src/examples/persistent_volume_framework.cpp fe2837cfffb6dd251ccb9c93197f623d0c55fb36

>   src/examples/test_framework.cpp a83766a92617d50eadd92ec55113e049a7290d03 
>   src/examples/test_http_framework.cpp cf6baed07c862ccade080618f33cc921fc09415d 
>   src/launcher/executor.cpp 5a5f95f04a6ce096079b67397cb324575409f795 
>   src/local/main.cpp 578b65efac1dd8ec201bfcc85de353ca6b867148 
>   src/master/main.cpp 84f3b0723fd3df02386c8072ded3cb42272cd065 
>   src/slave/container_loggers/lib_logrotate.hpp 8c5602da3e5ff7bcf758da61723b7a0ea00a6a6e

>   src/slave/container_loggers/logrotate.hpp 16d92322079a69d8e6bed7830623c62f345cd51c

>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 92b33111799cb4e1c8bc2051381e1254d701d95c

>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 79ee960aa70f8bd39f9bf639cc54f6395ff21ad5

>   src/slave/containerizer/mesos/launch.cpp 09733abc9b0ecc599efb114af8231c31c7ec6ffc 
>   src/slave/containerizer/mesos/mount.cpp dbd7853a43ee1402f2f91d933a657010efdd76b0 
>   src/slave/main.cpp a7ed669a0b6861d6ce8546dfafac849044a77eec 
> 
> Diff: https://reviews.apache.org/r/46824/diff/
> 
> 
> Testing
> -------
> 
> Tested on various platforms in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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