----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66954/#review202574 -----------------------------------------------------------
3rdparty/stout/include/stout/os/windows/io.hpp Lines 13-27 (patched) <https://reviews.apache.org/r/66954/#comment284389> Is this `os::internal` or is it `internal::windows`? I think because it's very Windows-specific that it's the latter, and therefore should probably be under `stout/internal/windows/overlapped.hpp`. 3rdparty/stout/include/stout/os/windows/io.hpp Lines 25-27 (patched) <https://reviews.apache.org/r/66954/#comment284405> Run `clang-format` on the whole file please :) 3rdparty/stout/include/stout/os/windows/io.hpp Lines 40 (patched) <https://reviews.apache.org/r/66954/#comment284399> Can you point the docs that explain this? (And maybe explain the parameters inline; for instance, we're creating a non-inheritable, auto-resetting, non-signaled, unnamed event, but I had to read the docs to figure that out.) 3rdparty/stout/include/stout/os/windows/io.hpp Lines 41 (patched) <https://reviews.apache.org/r/66954/#comment284388> I know Joseph would point this out: don't mention Mesos in Stout. 3rdparty/stout/include/stout/os/windows/io.hpp Lines 52-53 (patched) <https://reviews.apache.org/r/66954/#comment284391> Well that looks like voodoo. May want to quote those docs inline here: > This is done by specifying a valid event handle for the hEvent member of the OVERLAPPED structure, and setting its low-order bit. A valid event handle whose low-order bit is set keeps I/O completion from being queued to the completion port. 3rdparty/stout/include/stout/os/windows/io.hpp Lines 67-77 (patched) <https://reviews.apache.org/r/66954/#comment284402> You could replace `DWORD error = ::GetLastError();` with just `WindowsError error;` then check `error.code` in the conditional, and `return error` at the end. Just style though. 3rdparty/stout/include/stout/os/windows/io.hpp Lines 71 (patched) <https://reviews.apache.org/r/66954/#comment284401> `s/there no/there is no` 3rdparty/stout/include/stout/os/windows/io.hpp Lines 93 (patched) <https://reviews.apache.org/r/66954/#comment284409> I really wish that strongly typed enums had gotten rid of the need to do this. Alas :( 3rdparty/stout/include/stout/os/windows/io.hpp Lines 95-100 (patched) <https://reviews.apache.org/r/66954/#comment284410> Why the mix of `switch` and `if`? 3rdparty/stout/include/stout/os/windows/io.hpp Lines 95-170 (patched) <https://reviews.apache.org/r/66954/#comment284415> I think readability would be improved if you moved the `HANDLE` and `SOCKET` logic into two new functions that this function then calls appropriately. That way the top-level switch statement is confined to this function, and the two helper functions each only have one logical switch, instead of this function having nested switches. 3rdparty/stout/include/stout/os/windows/io.hpp Lines 122 (patched) <https://reviews.apache.org/r/66954/#comment284412> This reads funny. It's okay to implicitly cast Windows' `TRUE` to a bool. 3rdparty/stout/include/stout/os/windows/io.hpp Lines 140 (patched) <https://reviews.apache.org/r/66954/#comment284413> So this gets written to by `WSARecv`, but we throw it away? 3rdparty/stout/include/stout/os/windows/io.hpp Lines 187-189 (patched) <https://reviews.apache.org/r/66954/#comment284421> Maybe leave a `NOTE` that this specifically cannot reuse `os::read` and `os::write` because they can't handle a `HANDLE` that was opened in overlapped mode. Speaking of; what are those functions going to do if given an `int_fd` opened with overlapped semantics? 3rdparty/stout/include/stout/os/windows/io.hpp Lines 224 (patched) <https://reviews.apache.org/r/66954/#comment284419> Our type casts are funny. I assume this is to make it consistent with existing stout APIs? - Andrew Schwartzmeyer On May 4, 2018, 10:21 a.m., Akash Gupta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66954/ > ----------------------------------------------------------- > > (Updated May 4, 2018, 10:21 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, > John Kordich, Joseph Wu, and Radhika Jandhyala. > > > Bugs: MESOS-8670 > https://issues.apache.org/jira/browse/MESOS-8670 > > > Repository: mesos > > > Description > ------- > > Added a helper file for io, because `os::read/write` need to work on > both non-overlapped and overlapped files, so the logic has gotten > more complicated. The new file, `io.hpp`, has all the shared logic. > > > Diffs > ----- > > 3rdparty/stout/include/Makefile.am f2e60224d9c729497bbcfffbec989c4f355d8f85 > 3rdparty/stout/include/stout/os/windows/io.hpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/66954/diff/1/ > > > Testing > ------- > > > Thanks, > > Akash Gupta > >
