----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29528/#review69264 -----------------------------------------------------------
3rdparty/libprocess/include/process/socket.hpp <https://reviews.apache.org/r/29528/#comment113906> Does it make sense to have the default value here if the Socket::recv function already has a default? This seems error prone. 3rdparty/libprocess/src/socket.cpp <https://reviews.apache.org/r/29528/#comment113914> Should we use the Socket* pattern here as we did in process.cpp? The argument for this was to make it clear to people that we're keeping a copy of the socket alive until the operation is completed. In this case it is implicit. 3rdparty/libprocess/src/socket.cpp <https://reviews.apache.org/r/29528/#comment113918> Is this the correct use of Owned? Same as the comment above, we're implictly moving ownership between the callbacks here. If it is correct, can we add a comment at the implicit move site (l123) explaining this? 3rdparty/libprocess/src/socket.cpp <https://reviews.apache.org/r/29528/#comment113907> If we want to be explicit about the size this is fine. What do you think about introducing a static global for pagesize that is based on sysconf(_SC_PAGESIZE) (http://linux.die.net/man/2/getpagesize)? 3rdparty/libprocess/src/socket.cpp <https://reviews.apache.org/r/29528/#comment113909> const size_t chunk Can we put parens around the condition? I find it easier to read ternaries when any multi-piece section of A ? B : C is in parens. e.g. (A1 || A2) ? B : C. What do you think? 3rdparty/libprocess/src/socket.cpp <https://reviews.apache.org/r/29528/#comment113915> re comment on l85: new Socket(socket()) 3rdparty/libprocess/src/socket.cpp <https://reviews.apache.org/r/29528/#comment113916> Same implicit copy as l85. 3rdparty/libprocess/src/socket.cpp <https://reviews.apache.org/r/29528/#comment113919> Same as comment on line 87. 3rdparty/libprocess/src/socket.cpp <https://reviews.apache.org/r/29528/#comment113917> re l85: new Socket(socket())? - Joris Van Remoortere On Jan. 21, 2015, 5:42 p.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29528/ > ----------------------------------------------------------- > > (Updated Jan. 21, 2015, 5:42 p.m.) > > > Review request for mesos, Joris Van Remoortere and Niklas Nielsen. > > > Repository: mesos-git > > > Description > ------- > > See summary. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/socket.hpp > ddb9e365fc1e65a568bdac4973964df1ab8cc05e > 3rdparty/libprocess/src/socket.cpp 4b0f6bec8051f938812dbc90a7312e4082ea203f > > Diff: https://reviews.apache.org/r/29528/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
