----------------------------------------------------------- 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 > >
