You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alex Clemmer <cl...@gmail.com> on 2016/04/18 20:52:23 UTC
Review Request 46340: Stout:[1/2] Implement `os::waitpid`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46340/
-----------------------------------------------------------
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
Bugs: MESOS-4466
https://issues.apache.org/jira/browse/MESOS-4466
Repository: mesos
Description
-------
Stout:[1/2] Implement `os::waitpid`.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp a7f855dc9d0a87fe3b6d1611e7ae22e4d7cd7b6d
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
Diff: https://reviews.apache.org/r/46340/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 46340: Stout:[1/2] Implement `os::waitpid`.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46340/#review129767
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp (line 128)
<https://reviews.apache.org/r/46340/#comment193312>
spelling.
3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp (line 409)
<https://reviews.apache.org/r/46340/#comment193309>
grammar
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 254)
<https://reviews.apache.org/r/46340/#comment193323>
Why the `In plain English:`?
I think just the comment is sufficient.
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 261)
<https://reviews.apache.org/r/46340/#comment193324>
start the comment with a capital:
`: Check ...`
Missing some punctuations between `processes if not`?
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (lines 265 - 267)
<https://reviews.apache.org/r/46340/#comment193326>
Can you wrap the parameters one on each line?
Also leave a new line after an expression that doesn't fit on a single line.
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 269)
<https://reviews.apache.org/r/46340/#comment193328>
Can we use `stringify` here? I think we let some of these slip previously. Can we have a cleanup review that fixes these with `stringify`?
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (lines 306 - 308)
<https://reviews.apache.org/r/46340/#comment193330>
`so return 0` is deceptive. Can we say something like `return `None` to map to `waitpid` return of 0`?
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (lines 330 - 334)
<https://reviews.apache.org/r/46340/#comment193333>
This must have slipped in a merge elsewhere. Can you cut this in to its own review?
- Joris Van Remoortere
On April 18, 2016, 6:52 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46340/
> -----------------------------------------------------------
>
> (Updated April 18, 2016, 6:52 p.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Bugs: MESOS-4466
> https://issues.apache.org/jira/browse/MESOS-4466
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout:[1/2] Implement `os::waitpid`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp a7f855dc9d0a87fe3b6d1611e7ae22e4d7cd7b6d
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
>
> Diff: https://reviews.apache.org/r/46340/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 46340: Stout:[1/2] Implement `os::waitpid`.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46340/#review130013
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 254)
<https://reviews.apache.org/r/46340/#comment193625>
start with a capital.
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (lines 268 - 269)
<https://reviews.apache.org/r/46340/#comment193624>
new line.
- Joris Van Remoortere
On April 20, 2016, 9:51 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46340/
> -----------------------------------------------------------
>
> (Updated April 20, 2016, 9:51 p.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Bugs: MESOS-4466
> https://issues.apache.org/jira/browse/MESOS-4466
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout:[1/2] Implement `os::waitpid`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp a7f855dc9d0a87fe3b6d1611e7ae22e4d7cd7b6d
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
>
> Diff: https://reviews.apache.org/r/46340/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 46340: Stout:[1/2] Implement `os::waitpid`.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46340/
-----------------------------------------------------------
(Updated April 20, 2016, 9:51 p.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
Bugs: MESOS-4466
https://issues.apache.org/jira/browse/MESOS-4466
Repository: mesos
Description
-------
Stout:[1/2] Implement `os::waitpid`.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp a7f855dc9d0a87fe3b6d1611e7ae22e4d7cd7b6d
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
Diff: https://reviews.apache.org/r/46340/diff/
Testing
-------
Thanks,
Alex Clemmer