----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53460/#review157097 -----------------------------------------------------------
Fix it, then Ship it! 3rdparty/libprocess/include/process/address.hpp (line 69) <https://reviews.apache.org/r/53460/#comment227520> Ditto. 3rdparty/libprocess/include/process/address.hpp (line 80) <https://reviews.apache.org/r/53460/#comment227516> Use switch consistently as other functions? 3rdparty/libprocess/include/process/address.hpp (lines 80 - 81) <https://reviews.apache.org/r/53460/#comment227519> This won't compile on windows. Looks like AF_UNIX is not even defined on windows: https://msdn.microsoft.com/en-us/library/windows/desktop/ms740506(v=vs.85).aspx We should properly wrap UNIX with `#ifndef __WINDOWS__` 3rdparty/libprocess/include/process/address.hpp (line 108) <https://reviews.apache.org/r/53460/#comment227521> Wrap with `#ifndef __WINDOWS__` 3rdparty/libprocess/include/process/address.hpp (line 129) <https://reviews.apache.org/r/53460/#comment227531> +1. We should check on path length here. Probably introduce a 'create' function? 3rdparty/libprocess/include/process/address.hpp (lines 133 - 140) <https://reviews.apache.org/r/53460/#comment227530> +1 on using union as suggested by James. 3rdparty/libprocess/include/process/address.hpp (line 143) <https://reviews.apache.org/r/53460/#comment227551> Maybe you can store a `network::Address` here? 3rdparty/libprocess/include/process/address.hpp (line 167) <https://reviews.apache.org/r/53460/#comment227535> This is non-POD global constant which might cause trouble during global teardown. We should either use constexpr here (making sure Address's constructor is constexpr constructor), or use static method as we did before. 3rdparty/libprocess/include/process/address.hpp (line 169) <https://reviews.apache.org/r/53460/#comment227536> Ditto. 3rdparty/libprocess/include/process/address.hpp (lines 223 - 231) <https://reviews.apache.org/r/53460/#comment227528> +1 on using a union as suggested by James. 3rdparty/libprocess/include/process/socket.hpp (line 432) <https://reviews.apache.org/r/53460/#comment227553> inline is not necessary for template specification? Let's be consistent here (either use inline for all of them, or none of them). 3rdparty/libprocess/src/socket.cpp (lines 56 - 59) <https://reviews.apache.org/r/53460/#comment227568> `ifndef WINDOWS` 3rdparty/libprocess/src/tests/socket_tests.cpp (line 39) <https://reviews.apache.org/r/53460/#comment227569> Looks like this only works on Linux? should we use a socket path in temp dir. Maybe use TemporaryDirectoryTest fixture and use `path::join(sandbox.get(), "socket")` as the path? 3rdparty/libprocess/src/tests/ssl_tests.cpp (line 51) <https://reviews.apache.org/r/53460/#comment227570> wrap with ifdef WINDOWS - Jie Yu On Nov. 28, 2016, 6:15 a.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53460/ > ----------------------------------------------------------- > > (Updated Nov. 28, 2016, 6:15 a.m.) > > > Review request for mesos, Benjamin Mahler and Jie Yu. > > > Repository: mesos > > > Description > ------- > > Also added unix::Address. > > > Diffs > ----- > > 3rdparty/libprocess/Makefile.am 71319891a451bd1d565210dcce2fb61fc69e1f61 > 3rdparty/libprocess/include/process/address.hpp > 04e3155d65f476208fa852e83b79d173b66288fd > 3rdparty/libprocess/include/process/network.hpp > 52110667185370a4c92e2fa524819ab1f34bdec9 > 3rdparty/libprocess/include/process/socket.hpp > f798af7879546d71e8ef4a295c9cf489a70cb61f > 3rdparty/libprocess/include/process/ssl/gtest.hpp > 21a0fc45b55a368a21b3e616c751ab43eebd4902 > 3rdparty/libprocess/src/address.cpp PRE-CREATION > 3rdparty/libprocess/src/libevent_ssl_socket.cpp > 5c0929d3d9f5595bd2f343b98b899fd6b06a67b2 > 3rdparty/libprocess/src/poll_socket.cpp > eb7b48713edd30b545d7be95b5d51b0f71bd422a > 3rdparty/libprocess/src/process.cpp > e9a4bbb0b2410e0260d120b97e73972c94eb0f26 > 3rdparty/libprocess/src/socket.cpp 7f93168e1572f8669f67a4c5e6e5467259b7a407 > 3rdparty/libprocess/src/tests/socket_tests.cpp PRE-CREATION > 3rdparty/libprocess/src/tests/ssl_tests.cpp > 55c8c309571b1892b0acc4d766eda9bb98085a6f > 3rdparty/libprocess/src/tests/test_linkee.cpp > 1f6cfafcb73fd41ef350b13e3ac6023d78f16f5a > > Diff: https://reviews.apache.org/r/53460/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
