----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68642/#review208411 -----------------------------------------------------------
We should add a basic test for this, even if it just ensures that the result contains the elements 0, 1 and 2. 3rdparty/stout/include/stout/os/lsof.hpp Lines 16 (patched) <https://reviews.apache.org/r/68642/#comment292313> Is this really a double newline? I can't keep track, but this seems inconsistent with the other files in this patch. I looked in the current headers and found 3 variants of how many blank lines files like this should have :) 3rdparty/stout/include/stout/os/posix/lsof.hpp Lines 29 (patched) <https://reviews.apache.org/r/68642/#comment292315> Why hashset rather than vector? By definition there aren't any duplicates, so we don't need the overhead of hashset here? /cc @bmahler 3rdparty/stout/include/stout/os/posix/lsof.hpp Lines 35 (patched) <https://reviews.apache.org/r/68642/#comment292314> We don't actually need any #ifdefs here. On Linux, /dev/fd is a symlink to /proc/self/fd, so we can just unconditionally list /dev/fd and handle the error for platforms where this is missing. 3rdparty/stout/include/stout/os/posix/lsof.hpp Lines 46 (patched) <https://reviews.apache.org/r/68642/#comment292316> Add the fd string into the error message? 3rdparty/stout/include/stout/os/windows/lsof.hpp Lines 23 (patched) <https://reviews.apache.org/r/68642/#comment292317> In other case, we simply omitted the Windows version altogether, e.g. https://reviews.apache.org/r/68398/ /cc @andschwa - James Peach On Sept. 6, 2018, 1:25 a.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68642/ > ----------------------------------------------------------- > > (Updated Sept. 6, 2018, 1:25 a.m.) > > > Review request for mesos, Gilbert Song and James Peach. > > > Bugs: MESOS-9152 > https://issues.apache.org/jira/browse/MESOS-9152 > > > Repository: mesos > > > Description > ------- > > Added `lsof()` into stout. > > > Diffs > ----- > > 3rdparty/stout/include/Makefile.am a9c61fcba253a811494cdcdb0afb3d3a018f4585 > 3rdparty/stout/include/stout/os.hpp > aee041891b7e7ff93a0b1ac31019a7a3d4eae962 > 3rdparty/stout/include/stout/os/lsof.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/posix/lsof.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/windows/lsof.hpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/68642/diff/1/ > > > Testing > ------- > > > Thanks, > > Qian Zhang > >
