You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Cong Wang <xi...@gmail.com> on 2016/02/11 01:38:55 UTC
Review Request 43461: Used C++11 for loop to iterate std list/set.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43461/
-----------------------------------------------------------
Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone.
Repository: mesos
Description
-------
Used C++11 for loop to iterate std list/set.
Diffs
-----
3rdparty/libprocess/include/process/future.hpp 77655f22ad4ce6e81fd46bcdebbf2a82786e8f49
Diff: https://reviews.apache.org/r/43461/diff/
Testing
-------
make check
Thanks,
Cong Wang
Re: Review Request 43461: Used C++11 for loop to iterate std list/set.
Posted by Michael Park <mp...@apache.org>.
> On Feb. 24, 2016, 7:29 p.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/future.hpp, line 791
> > <https://reviews.apache.org/r/43461/diff/2/?file=1268386#file1268386line791>
> >
> > * Mesos currently uses `foreach` from stout rather than range-based`for`.
> > * Mesos generally prefers explicit types than `auto`.
> >
> > ```
> > foreach (Future<T> future, futures) {
> > ...
> > }
> > ```
> >
> > Here and below.
>
> Cong Wang wrote:
> Is that a strict rule? At least src/tests/hierarchical_allocator_tests.cpp uses for(auto...) too.
Yeah, those shouldn't have made it into the codebase for consistency. The migration strategy is being discussed/tracked here: [MESOS-3214](https://issues.apache.org/jira/browse/MESOS-3214).
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43461/#review120550
-----------------------------------------------------------
On Feb. 24, 2016, 7:19 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43461/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2016, 7:19 p.m.)
>
>
> Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used C++11 for loop to iterate std list/set.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp 77655f22ad4ce6e81fd46bcdebbf2a82786e8f49
>
> Diff: https://reviews.apache.org/r/43461/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 43461: Used C++11 for loop to iterate std list/set.
Posted by Cong Wang <xi...@gmail.com>.
> On Feb. 24, 2016, 7:29 p.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/future.hpp, line 791
> > <https://reviews.apache.org/r/43461/diff/2/?file=1268386#file1268386line791>
> >
> > * Mesos currently uses `foreach` from stout rather than range-based`for`.
> > * Mesos generally prefers explicit types than `auto`.
> >
> > ```
> > foreach (Future<T> future, futures) {
> > ...
> > }
> > ```
> >
> > Here and below.
Is that a strict rule? At least src/tests/hierarchical_allocator_tests.cpp uses for(auto...) too.
- Cong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43461/#review120550
-----------------------------------------------------------
On Feb. 24, 2016, 7:19 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43461/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2016, 7:19 p.m.)
>
>
> Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used C++11 for loop to iterate std list/set.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp 77655f22ad4ce6e81fd46bcdebbf2a82786e8f49
>
> Diff: https://reviews.apache.org/r/43461/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 43461: Used C++11 for loop to iterate std list/set.
Posted by Cong Wang <xi...@gmail.com>.
> On Feb. 24, 2016, 7:29 p.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/future.hpp, line 791
> > <https://reviews.apache.org/r/43461/diff/2/?file=1268386#file1268386line791>
> >
> > * Mesos currently uses `foreach` from stout rather than range-based`for`.
> > * Mesos generally prefers explicit types than `auto`.
> >
> > ```
> > foreach (Future<T> future, futures) {
> > ...
> > }
> > ```
> >
> > Here and below.
>
> Cong Wang wrote:
> Is that a strict rule? At least src/tests/hierarchical_allocator_tests.cpp uses for(auto...) too.
>
> Michael Park wrote:
> Yeah, those shouldn't have made it into the codebase for consistency. The migration strategy is being discussed/tracked here: [MESOS-3214](https://issues.apache.org/jira/browse/MESOS-3214).
OK. I will use foreach before we migrate to C++11 for loop.
- Cong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43461/#review120550
-----------------------------------------------------------
On Feb. 24, 2016, 7:19 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43461/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2016, 7:19 p.m.)
>
>
> Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used C++11 for loop to iterate std list/set.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp 77655f22ad4ce6e81fd46bcdebbf2a82786e8f49
>
> Diff: https://reviews.apache.org/r/43461/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 43461: Used C++11 for loop to iterate std list/set.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43461/#review120550
-----------------------------------------------------------
Thanks for the cleanup!
3rdparty/libprocess/include/process/future.hpp (line 791)
<https://reviews.apache.org/r/43461/#comment182020>
* Mesos currently uses `foreach` from stout rather than range-based`for`.
* Mesos generally prefers explicit types than `auto`.
```
foreach (Future<T> future, futures) {
...
}
```
Here and below.
- Michael Park
On Feb. 24, 2016, 7:19 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43461/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2016, 7:19 p.m.)
>
>
> Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used C++11 for loop to iterate std list/set.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp 77655f22ad4ce6e81fd46bcdebbf2a82786e8f49
>
> Diff: https://reviews.apache.org/r/43461/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 43461: Used foreach loop to iterate std list/set.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43461/#review120677
-----------------------------------------------------------
Ship it!
Ship It!
- Michael Park
On Feb. 25, 2016, 4:11 a.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43461/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2016, 4:11 a.m.)
>
>
> Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used foreach loop to iterate std list/set.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp 77655f22ad4ce6e81fd46bcdebbf2a82786e8f49
>
> Diff: https://reviews.apache.org/r/43461/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 43461: Used foreach loop to iterate std list/set.
Posted by Cong Wang <xi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43461/
-----------------------------------------------------------
(Updated Feb. 25, 2016, 4:11 a.m.)
Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone.
Changes
-------
Address review comments
Summary (updated)
-----------------
Used foreach loop to iterate std list/set.
Repository: mesos
Description (updated)
-------
Used foreach loop to iterate std list/set.
Diffs (updated)
-----
3rdparty/libprocess/include/process/future.hpp 77655f22ad4ce6e81fd46bcdebbf2a82786e8f49
Diff: https://reviews.apache.org/r/43461/diff/
Testing
-------
make check
Thanks,
Cong Wang
Re: Review Request 43461: Used C++11 for loop to iterate std list/set.
Posted by Michael Park <mp...@apache.org>.
> On Feb. 24, 2016, 7:30 p.m., Vinod Kone wrote:
> > LGTM. Though I'm not sure why we used iterators instead of foreach in the first place. cc @mcypark
Because it was written before `foreach` was introduced to stout. The last of the `for` loops here was authored 2011-10-31 and `foreach` was introduced 2012-07-11.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43461/#review120552
-----------------------------------------------------------
On Feb. 24, 2016, 7:19 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43461/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2016, 7:19 p.m.)
>
>
> Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used C++11 for loop to iterate std list/set.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp 77655f22ad4ce6e81fd46bcdebbf2a82786e8f49
>
> Diff: https://reviews.apache.org/r/43461/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 43461: Used C++11 for loop to iterate std list/set.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43461/#review120552
-----------------------------------------------------------
Ship it!
LGTM. Though I'm not sure why we used iterators instead of foreach in the first place. cc @mcypark
- Vinod Kone
On Feb. 24, 2016, 7:19 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43461/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2016, 7:19 p.m.)
>
>
> Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used C++11 for loop to iterate std list/set.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp 77655f22ad4ce6e81fd46bcdebbf2a82786e8f49
>
> Diff: https://reviews.apache.org/r/43461/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 43461: Used C++11 for loop to iterate std list/set.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43461/#review120564
-----------------------------------------------------------
3rdparty/libprocess/include/process/future.hpp (line 791)
<https://reviews.apache.org/r/43461/#comment182031>
Why do we need to create a copy here now?
- Benjamin Bannier
On Feb. 24, 2016, 8:19 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43461/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2016, 8:19 p.m.)
>
>
> Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used C++11 for loop to iterate std list/set.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp 77655f22ad4ce6e81fd46bcdebbf2a82786e8f49
>
> Diff: https://reviews.apache.org/r/43461/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 43461: Used C++11 for loop to iterate std list/set.
Posted by Cong Wang <xi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43461/
-----------------------------------------------------------
(Updated Feb. 24, 2016, 7:19 p.m.)
Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone.
Changes
-------
rebase
Repository: mesos
Description
-------
Used C++11 for loop to iterate std list/set.
Diffs (updated)
-----
3rdparty/libprocess/include/process/future.hpp 77655f22ad4ce6e81fd46bcdebbf2a82786e8f49
Diff: https://reviews.apache.org/r/43461/diff/
Testing
-------
make check
Thanks,
Cong Wang
Re: Review Request 43461: Used C++11 for loop to iterate std list/set.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43461/#review118809
-----------------------------------------------------------
Patch looks great!
Reviews applied: [43461]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Feb. 11, 2016, 12:38 a.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43461/
> -----------------------------------------------------------
>
> (Updated Feb. 11, 2016, 12:38 a.m.)
>
>
> Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used C++11 for loop to iterate std list/set.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp 77655f22ad4ce6e81fd46bcdebbf2a82786e8f49
>
> Diff: https://reviews.apache.org/r/43461/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>