----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43985/#review129633 -----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/sendfile.hpp (lines 33 - 34) <https://reviews.apache.org/r/43985/#comment193090> (1) `s/On error, On error,/On error,/` (2) Remove the extra space in `// Try`. i.e., `// Try` 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/sendfile.hpp (lines 39 - 40) <https://reviews.apache.org/r/43985/#comment193089> ``` inline Try<ssize_t, SocketError> sendfile( int s, int fd, off_t offset, size_t length) { ``` 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/sendfile.hpp (line 45) <https://reviews.apache.org/r/43985/#comment193088> `s/send/sent/` 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/sendfile.hpp (line 46) <https://reviews.apache.org/r/43985/#comment193084> (1) `s/return return/return/` (2) Based on other syscalls, it seems like we should just do `return SocketError();`. e.g., `close.hpp`, `chroot.hpp`, `fnctl.hpp`, etc. Here and below. 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp (lines 16 - 17) <https://reviews.apache.org/r/43985/#comment193103> When we fork to have separate implementation for `POSIX` and `Windows`, we should keep them completely separate. That is, even though `<stout/error.hpp>` is used in both implementations, they should include them individually. For example, starting from `3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/sendfile.hpp` we can't find where `ErrnoError` came from. 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp (lines 19 - 24) <https://reviews.apache.org/r/43985/#comment193081> Let's consolidate this with the `process::network::SocketError` in `3rdparty/libprocess/include/process/network.hpp`. For now, I think `stout/error.hpp` is the best place to put it. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/sendfile.hpp (lines 16 - 17) <https://reviews.apache.org/r/43985/#comment193091> Add a newline in between. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/sendfile.hpp (line 23) <https://reviews.apache.org/r/43985/#comment193092> Backquotes around `Try<ssize_t, WindowsSocketError>`. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/sendfile.hpp (lines 24 - 25) <https://reviews.apache.org/r/43985/#comment193094> ``` inline Try<ssize_t, SocketError> sendfile( int s, int fd, off_t offset, size_t length) { ``` 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/sendfile.hpp (line 27) <https://reviews.apache.org/r/43985/#comment193095> `s/ will/, it will/` 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/sendfile.hpp (lines 28 - 30) <https://reviews.apache.org/r/43985/#comment193100> Please use the correct C++ casts here for the casts to `HANDLE`, `LONG`. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/sendfile.hpp (line 30) <https://reviews.apache.org/r/43985/#comment193096> `hight_Part`? did we mean `high_part`? Just `high` seems fine too. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/sendfile.hpp (line 31) <https://reviews.apache.org/r/43985/#comment193101> It looks like the second parameter of `SetFilePointer` is `LONG`, not `DWORD`. Why do we cast to `DWORD`? Also, please use C++ cast. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/sendfile.hpp (line 33) <https://reviews.apache.org/r/43985/#comment193097> Should return `SocketError` here (I know they're the same, but still). 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/sendfile.hpp (line 39) <https://reviews.apache.org/r/43985/#comment193098> Should return `SocketError` here (I know they're the same, but still). - Michael Park On April 16, 2016, 1:10 a.m., Daniel Pravat wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43985/ > ----------------------------------------------------------- > > (Updated April 16, 2016, 1:10 a.m.) > > > Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van > Remoortere, and Michael Park. > > > Repository: mesos > > > Description > ------- > > Windows: [1/3] Implemented `sendfile`. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/sendfile.hpp > 293f82f6730551491504721e0af28e9537540db1 > 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp > f3197679a9c22b37acae003262e5c6d21e110f56 > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/sendfile.hpp > 750ca6749cb028703ed2fb5dec2aac6c5dabea0d > 3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp > a7f855dc9d0a87fe3b6d1611e7ae22e4d7cd7b6d > > Diff: https://reviews.apache.org/r/43985/diff/ > > > Testing > ------- > > Build Windows,OSX > > > Thanks, > > Daniel Pravat > >
