----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67201/#review203382 -----------------------------------------------------------
3rdparty/stout/include/stout/internal/windows/inherit.hpp Lines 29 (patched) <https://reviews.apache.org/r/67201/#comment285575> Prefer using `*` over the Hungarian-style prefix `LP`, so `PROC_THREAD_ATTRIBUTE_LIST*` instead of `LPPROC_THREAD_ATTRIBUTE_LIST`. 3rdparty/stout/include/stout/internal/windows/inherit.hpp Lines 33-34 (patched) <https://reviews.apache.org/r/67201/#comment285572> Nit on style: Since we're in C++ code, I try to prefix global functions (like system APIs) with `::`, and prefer `nullptr` to `NULL`. 3rdparty/stout/include/stout/internal/windows/inherit.hpp Lines 34 (patched) <https://reviews.apache.org/r/67201/#comment285573> What do the second and third arguments represent? 3rdparty/stout/include/stout/internal/windows/inherit.hpp Lines 41-43 (patched) <https://reviews.apache.org/r/67201/#comment285571> We should probably use a `std::unique_ptr<PROC_THREAD_ATTRIBUTE_LIST>` here for RAII. 3rdparty/stout/include/stout/internal/windows/inherit.hpp Lines 50 (patched) <https://reviews.apache.org/r/67201/#comment285574> RAII takes care of this ;) I noticed below we also called `DeleteProcThreadAttributeList`. Under what conditions do we `DeleteProcThreadAttributeList` and `free`, and when do we just `free`? 3rdparty/stout/include/stout/internal/windows/inherit.hpp Lines 58 (patched) <https://reviews.apache.org/r/67201/#comment285576> Prefer a static_cast or similar over C-style casting. 3rdparty/stout/include/stout/os/windows/shell.hpp Line 269 (original), 270 (patched) <https://reviews.apache.org/r/67201/#comment285581> Is there a `W` version of this, or is that redundant...? I don't know. 3rdparty/stout/include/stout/os/windows/shell.hpp Lines 279-291 (original), 283-306 (patched) <https://reviews.apache.org/r/67201/#comment285579> (Old comment: Since the size is `constexpr` (it's always 3), I don't think we need to malloc on the heap. We can just use `std::array<HANDLE, 3>`.) Actually, I don't think you even need `handle_array` at all. `pipes` is already a `std::array<int_fd, 3>` (and `int_fd` in this case is `HANDLE`). You can use the [data()](http://en.cppreference.com/w/cpp/container/array/data) member of `std::array` to get access to the underlying `HANDLE[]`. 3rdparty/stout/include/stout/os/windows/shell.hpp Lines 339-341 (patched) <https://reviews.apache.org/r/67201/#comment285580> RAII and you can delete this! - Andrew Schwartzmeyer On May 17, 2018, 3:18 p.m., Radhika Jandhyala wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67201/ > ----------------------------------------------------------- > > (Updated May 17, 2018, 3:18 p.m.) > > > Review request for mesos and Andrew Schwartzmeyer. > > > Bugs: MESOS-8926 > https://issues.apache.org/jira/browse/MESOS-8926 > > > Repository: mesos > > > Description > ------- > > Whitelist handles that can be inherited by a child process. > > > Diffs > ----- > > 3rdparty/stout/include/stout/internal/windows/inherit.hpp > 7dbde820e775cbaeb8db4bc4559ab432903e75ea > 3rdparty/stout/include/stout/os/windows/shell.hpp > 8da612af2888ff4d4d458ea5b16cdb08779b6f4c > > > Diff: https://reviews.apache.org/r/67201/diff/1/ > > > Testing > ------- > > > Thanks, > > Radhika Jandhyala > >
