You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by haosdent huang <ha...@gmail.com> on 2015/05/09 21:42:16 UTC
Review Request 34018: Update existing lambdas to meet style guide
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34018/
-----------------------------------------------------------
Review request for mesos.
Repository: mesos
Description
-------
According Joris advice, only replace the 'lambda::bind' expressions match these rules:
1. Binds to a static function without any side-effects.
2. Is self contained (i.e. does not rely on contextual parameters)
3. Does not bind in any arguments. (i.e. only uses lambda::_N for arguments)
4. Is only called in 1 place OR is so small that it is ok to repeat the code.
Diffs
-----
src/linux/cgroups.cpp df3211a0c25d7a16f36814886d14f81caaef2b9c
src/log/network.hpp 7c74a55cc2a71fa2acd207605f972e7fbd203be4
src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
src/slave/containerizer/isolators/cgroups/mem.cpp 2c218b2b83cf42f54dbc7ec4c2ba8960b6e194de
src/slave/containerizer/isolators/network/port_mapping.cpp a4abaff30bb4646b1b1edfdbbc243c9e3f6851df
src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603
Diff: https://reviews.apache.org/r/34018/diff/
Testing
-------
make check
Thanks,
haosdent huang
Re: Review Request 34018: Update existing lambdas to meet style guide
Posted by haosdent huang <ha...@gmail.com>.
> On May 16, 2015, 8:50 p.m., Joris Van Remoortere wrote:
> > src/log/network.hpp, lines 434-440
> > <https://reviews.apache.org/r/34018/diff/2/?file=954649#file954649line434>
> >
> > It looks like we missed a nice example for this in the style guide! (the lambda not being the first function parameter argument).
> >
> > What do you think of re-aligning the code like this:
> >
> > ```
> > process::collect(futures)
> > .after(
> > Seconds(5),
> > [](process::Future<std::list<Option<std::string>>> datas) {
> > // Handling time outs when collecting membership data. For
> > // now, a timeout is treated as a failure.
> > datas.discard();
> > return process::Failure("Timed out");
> > })
> > .onAny(executor.defer(lambda::bind(&This::collected, this, lambda::_1)));
> > ```
> >
> > Notice:
> > 1) I put the arguments to `after()` each on their own line, indented 4 spaces deeper.
> > 2) I re-wrapped the comment to the 70 character boundary.
> > 3) I got rid of the white-space between the closing brackets: `> > >` -> `>>>`. Yay newer compilers :-)
Nice, could not agree more. How about add a example to the document which describe the style when lambda not being the first parameter.
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34018/#review84042
-----------------------------------------------------------
On May 17, 2015, 10:11 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34018/
> -----------------------------------------------------------
>
> (Updated May 17, 2015, 10:11 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Bugs: MESOS-2670
> https://issues.apache.org/jira/browse/MESOS-2670
>
>
> Repository: mesos
>
>
> Description
> -------
>
> According Joris advice, replace the 'lambda::bind' expressions match these rules:
>
> 1. Binds to a static function without any side-effects.
> 2. Is self contained (i.e. does not rely on contextual parameters)
> 3. Does not bind in any arguments. (i.e. only uses lambda::_N for arguments)
> 4. Is only called in 1 place OR is so small that it is ok to repeat the code.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp df3211a0c25d7a16f36814886d14f81caaef2b9c
> src/log/network.hpp 7c74a55cc2a71fa2acd207605f972e7fbd203be4
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
> src/slave/containerizer/isolators/cgroups/mem.cpp 2c218b2b83cf42f54dbc7ec4c2ba8960b6e194de
> src/slave/containerizer/isolators/network/port_mapping.cpp a4abaff30bb4646b1b1edfdbbc243c9e3f6851df
> src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a
>
> Diff: https://reviews.apache.org/r/34018/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 34018: Update existing lambdas to meet style guide
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34018/#review84042
-----------------------------------------------------------
Hey Haosdent,
These look great.
Just 1 suggestion for cleaning up the style while we're touching these lines of code :-)
src/log/network.hpp
<https://reviews.apache.org/r/34018/#comment135174>
It looks like we missed a nice example for this in the style guide! (the lambda not being the first function parameter argument).
What do you think of re-aligning the code like this:
```
process::collect(futures)
.after(
Seconds(5),
[](process::Future<std::list<Option<std::string>>> datas) {
// Handling time outs when collecting membership data. For
// now, a timeout is treated as a failure.
datas.discard();
return process::Failure("Timed out");
})
.onAny(executor.defer(lambda::bind(&This::collected, this, lambda::_1)));
```
Notice:
1) I put the arguments to `after()` each on their own line, indented 4 spaces deeper.
2) I re-wrapped the comment to the 70 character boundary.
3) I got rid of the white-space between the closing brackets: `> > >` -> `>>>`. Yay newer compilers :-)
- Joris Van Remoortere
On May 9, 2015, 7:52 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34018/
> -----------------------------------------------------------
>
> (Updated May 9, 2015, 7:52 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Bugs: MESOS-2670
> https://issues.apache.org/jira/browse/MESOS-2670
>
>
> Repository: mesos
>
>
> Description
> -------
>
> According Joris advice, replace the 'lambda::bind' expressions match these rules:
>
> 1. Binds to a static function without any side-effects.
> 2. Is self contained (i.e. does not rely on contextual parameters)
> 3. Does not bind in any arguments. (i.e. only uses lambda::_N for arguments)
> 4. Is only called in 1 place OR is so small that it is ok to repeat the code.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp df3211a0c25d7a16f36814886d14f81caaef2b9c
> src/log/network.hpp 7c74a55cc2a71fa2acd207605f972e7fbd203be4
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
> src/slave/containerizer/isolators/cgroups/mem.cpp 2c218b2b83cf42f54dbc7ec4c2ba8960b6e194de
> src/slave/containerizer/isolators/network/port_mapping.cpp a4abaff30bb4646b1b1edfdbbc243c9e3f6851df
> src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603
>
> Diff: https://reviews.apache.org/r/34018/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 34018: Update existing lambdas to meet style guide
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34018/#review83163
-----------------------------------------------------------
Patch looks great!
Reviews applied: [34017, 34018]
All tests passed.
- Mesos ReviewBot
On May 9, 2015, 7:52 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34018/
> -----------------------------------------------------------
>
> (Updated May 9, 2015, 7:52 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Bugs: MESOS-2670
> https://issues.apache.org/jira/browse/MESOS-2670
>
>
> Repository: mesos
>
>
> Description
> -------
>
> According Joris advice, replace the 'lambda::bind' expressions match these rules:
>
> 1. Binds to a static function without any side-effects.
> 2. Is self contained (i.e. does not rely on contextual parameters)
> 3. Does not bind in any arguments. (i.e. only uses lambda::_N for arguments)
> 4. Is only called in 1 place OR is so small that it is ok to repeat the code.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp df3211a0c25d7a16f36814886d14f81caaef2b9c
> src/log/network.hpp 7c74a55cc2a71fa2acd207605f972e7fbd203be4
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
> src/slave/containerizer/isolators/cgroups/mem.cpp 2c218b2b83cf42f54dbc7ec4c2ba8960b6e194de
> src/slave/containerizer/isolators/network/port_mapping.cpp a4abaff30bb4646b1b1edfdbbc243c9e3f6851df
> src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603
>
> Diff: https://reviews.apache.org/r/34018/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 34018: Update existing lambdas to meet style guide
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34018/#review84061
-----------------------------------------------------------
Patch looks great!
Reviews applied: [34017, 34018]
All tests passed.
- Mesos ReviewBot
On May 17, 2015, 10:11 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34018/
> -----------------------------------------------------------
>
> (Updated May 17, 2015, 10:11 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Bugs: MESOS-2670
> https://issues.apache.org/jira/browse/MESOS-2670
>
>
> Repository: mesos
>
>
> Description
> -------
>
> According Joris advice, replace the 'lambda::bind' expressions match these rules:
>
> 1. Binds to a static function without any side-effects.
> 2. Is self contained (i.e. does not rely on contextual parameters)
> 3. Does not bind in any arguments. (i.e. only uses lambda::_N for arguments)
> 4. Is only called in 1 place OR is so small that it is ok to repeat the code.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp df3211a0c25d7a16f36814886d14f81caaef2b9c
> src/log/network.hpp 7c74a55cc2a71fa2acd207605f972e7fbd203be4
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
> src/slave/containerizer/isolators/cgroups/mem.cpp 2c218b2b83cf42f54dbc7ec4c2ba8960b6e194de
> src/slave/containerizer/isolators/network/port_mapping.cpp a4abaff30bb4646b1b1edfdbbc243c9e3f6851df
> src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a
>
> Diff: https://reviews.apache.org/r/34018/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 34018: Update existing lambdas to meet style guide
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34018/#review84222
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Hindman
On May 17, 2015, 10:11 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34018/
> -----------------------------------------------------------
>
> (Updated May 17, 2015, 10:11 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Bugs: MESOS-2670
> https://issues.apache.org/jira/browse/MESOS-2670
>
>
> Repository: mesos
>
>
> Description
> -------
>
> According Joris advice, replace the 'lambda::bind' expressions match these rules:
>
> 1. Binds to a static function without any side-effects.
> 2. Is self contained (i.e. does not rely on contextual parameters)
> 3. Does not bind in any arguments. (i.e. only uses lambda::_N for arguments)
> 4. Is only called in 1 place OR is so small that it is ok to repeat the code.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp df3211a0c25d7a16f36814886d14f81caaef2b9c
> src/log/network.hpp 7c74a55cc2a71fa2acd207605f972e7fbd203be4
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
> src/slave/containerizer/isolators/cgroups/mem.cpp 2c218b2b83cf42f54dbc7ec4c2ba8960b6e194de
> src/slave/containerizer/isolators/network/port_mapping.cpp a4abaff30bb4646b1b1edfdbbc243c9e3f6851df
> src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a
>
> Diff: https://reviews.apache.org/r/34018/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 34018: Update existing lambdas to meet style guide
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34018/
-----------------------------------------------------------
(Updated May 17, 2015, 10:11 a.m.)
Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
Bugs: MESOS-2670
https://issues.apache.org/jira/browse/MESOS-2670
Repository: mesos
Description
-------
According Joris advice, replace the 'lambda::bind' expressions match these rules:
1. Binds to a static function without any side-effects.
2. Is self contained (i.e. does not rely on contextual parameters)
3. Does not bind in any arguments. (i.e. only uses lambda::_N for arguments)
4. Is only called in 1 place OR is so small that it is ok to repeat the code.
Diffs (updated)
-----
src/linux/cgroups.cpp df3211a0c25d7a16f36814886d14f81caaef2b9c
src/log/network.hpp 7c74a55cc2a71fa2acd207605f972e7fbd203be4
src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
src/slave/containerizer/isolators/cgroups/mem.cpp 2c218b2b83cf42f54dbc7ec4c2ba8960b6e194de
src/slave/containerizer/isolators/network/port_mapping.cpp a4abaff30bb4646b1b1edfdbbc243c9e3f6851df
src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a
Diff: https://reviews.apache.org/r/34018/diff/
Testing
-------
make check
Thanks,
haosdent huang
Re: Review Request 34018: Update existing lambdas to meet style guide
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34018/
-----------------------------------------------------------
(Updated May 9, 2015, 7:52 p.m.)
Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
Bugs: MESOS-2670
https://issues.apache.org/jira/browse/MESOS-2670
Repository: mesos
Description (updated)
-------
According Joris advice, replace the 'lambda::bind' expressions match these rules:
1. Binds to a static function without any side-effects.
2. Is self contained (i.e. does not rely on contextual parameters)
3. Does not bind in any arguments. (i.e. only uses lambda::_N for arguments)
4. Is only called in 1 place OR is so small that it is ok to repeat the code.
Diffs
-----
src/linux/cgroups.cpp df3211a0c25d7a16f36814886d14f81caaef2b9c
src/log/network.hpp 7c74a55cc2a71fa2acd207605f972e7fbd203be4
src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
src/slave/containerizer/isolators/cgroups/mem.cpp 2c218b2b83cf42f54dbc7ec4c2ba8960b6e194de
src/slave/containerizer/isolators/network/port_mapping.cpp a4abaff30bb4646b1b1edfdbbc243c9e3f6851df
src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603
Diff: https://reviews.apache.org/r/34018/diff/
Testing
-------
make check
Thanks,
haosdent huang
Re: Review Request 34018: Update existing lambdas to meet style guide
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34018/
-----------------------------------------------------------
(Updated May 9, 2015, 7:50 p.m.)
Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
Bugs: MESOS-2670
https://issues.apache.org/jira/browse/MESOS-2670
Repository: mesos
Description
-------
According Joris advice, only replace the 'lambda::bind' expressions match these rules:
1. Binds to a static function without any side-effects.
2. Is self contained (i.e. does not rely on contextual parameters)
3. Does not bind in any arguments. (i.e. only uses lambda::_N for arguments)
4. Is only called in 1 place OR is so small that it is ok to repeat the code.
Diffs (updated)
-----
src/linux/cgroups.cpp df3211a0c25d7a16f36814886d14f81caaef2b9c
src/log/network.hpp 7c74a55cc2a71fa2acd207605f972e7fbd203be4
src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
src/slave/containerizer/isolators/cgroups/mem.cpp 2c218b2b83cf42f54dbc7ec4c2ba8960b6e194de
src/slave/containerizer/isolators/network/port_mapping.cpp a4abaff30bb4646b1b1edfdbbc243c9e3f6851df
src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603
Diff: https://reviews.apache.org/r/34018/diff/
Testing
-------
make check
Thanks,
haosdent huang