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

Reply via email to