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




3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (line 164)
<https://reviews.apache.org/r/46013/#comment191997>

    I assume "disappear" means "between enumeration and now". Can you expand 
this comment a bit please?



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 270)
<https://reviews.apache.org/r/46013/#comment192018>

    Doesn't follow stout naming convention (and can be mistaken for a WinAPI 
function).



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 272)
<https://reviews.apache.org/r/46013/#comment192025>

    Not entirely sure we need this output arg



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 286)
<https://reviews.apache.org/r/46013/#comment192020>

    spacing



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 289)
<https://reviews.apache.org/r/46013/#comment192023>

    Comparing a HANDLE to NULL is a risky thing to do. I don't think MSDN 
defines NULL (which is defined as 0 in WinAPI) as an invalid handle value. We 
might just trigger a false negative here. Any HANDLE result should only be 
tested against INVALID_HANDLE_VALUE.



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 294)
<https://reviews.apache.org/r/46013/#comment192024>

    I think we have some sort of safe_handle type by now, right?



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 304)
<https://reviews.apache.org/r/46013/#comment192027>

    This can probably be scoped to each code block. No need to define it here, 
just creates confusion.



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 339)
<https://reviews.apache.org/r/46013/#comment192028>

    These can all be on the same line.



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 372)
<https://reviews.apache.org/r/46013/#comment192015>

    I don't think we should be using this flag. The fact that FindProcess 
returned an error is a solid indication that something is wrong.



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 382)
<https://reviews.apache.org/r/46013/#comment192019>

    Can you qualify these calls with `::` please?



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 383)
<https://reviews.apache.org/r/46013/#comment192009>

    Can you document this flag a bit please? GetProcessTimes requires 
PROCESS_QUERY_LIMITED_INFORMATION, while GetProcessMemoryInfo requires the 
additional PROCESS_VM_READ.



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 399)
<https://reviews.apache.org/r/46013/#comment192011>

    Extra newline?



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 419)
<https://reviews.apache.org/r/46013/#comment192012>

    Newline



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 423)
<https://reviews.apache.org/r/46013/#comment192007>

    I don't think these can be negative, consider using ULARGE_INTEGER instead.



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 429)
<https://reviews.apache.org/r/46013/#comment192008>

    Do you need system_time and user_time? I think these can be inlined below.


- Alex Naparu


On April 11, 2016, 7:19 p.m., Alex Clemmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> -----------------------------------------------------------
> 
> (Updated April 11, 2016, 7:19 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4471
>     https://issues.apache.org/jira/browse/MESOS-4471
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Stout: Implemented `os::processes` on Windows.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> edaa76a5322d0bf60b7172405aa754b5aca95458 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> c48106e5905e3be0faeba7177ef534766089faff 
> 
> Diff: https://reviews.apache.org/r/46013/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Reply via email to