----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31930/#review76931 -----------------------------------------------------------
Ship it! LGTM. 3rdparty/libprocess/include/process/http.hpp <https://reviews.apache.org/r/31930/#comment124657> Instead of keeping a reference to Data, wouldn't be simpler and more straitforward to keep a Pipe instance (as Pipe is already copyable) here? You can avoid the forward declarations as well. ``` class Pipe { public: class Reader { public: ... private: // Necessary for accessing pipe.data. friend class Pipe; Reader(const Pipe& _pipe) : pipe(_pipe) {} Pipe pipe; }; }; ``` 3rdparty/libprocess/include/process/http.hpp <https://reviews.apache.org/r/31930/#comment124725> Are you exposing this function only for testing? If yes, please add a comment. 3rdparty/libprocess/include/process/http.hpp <https://reviews.apache.org/r/31930/#comment124662> Ditto. 3rdparty/libprocess/src/http.cpp <https://reviews.apache.org/r/31930/#comment124656> This can be simplified as: ``` return Pipe::Reader(*this); ``` 3rdparty/libprocess/src/http.cpp <https://reviews.apache.org/r/31930/#comment124722> Can you add an extra parathesis here. I am not super familar with the operator priorities:) 3rdparty/libprocess/src/process.cpp <https://reviews.apache.org/r/31930/#comment124728> Does this comment still apply? Looks like we cannot write empty string. Maybe a CHECK? - Jie Yu On March 16, 2015, 11:49 p.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31930/ > ----------------------------------------------------------- > > (Updated March 16, 2015, 11:49 p.m.) > > > Review request for mesos, Benjamin Hindman, Jie Yu, and Joris Van Remoortere. > > > Bugs: MESOS-2438 > https://issues.apache.org/jira/browse/MESOS-2438 > > > Repository: mesos > > > Description > ------- > > See MESOS-2438 for the motivation. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/http.hpp > 10143fdbd5eb43f968c41957a2335f96a097d82e > 3rdparty/libprocess/src/http.cpp 7c0cee404645e1dd1a9253f85883aefa29a5f999 > 3rdparty/libprocess/src/process.cpp > e7b029ba97e640c2102548c190ba62b30602f43d > 3rdparty/libprocess/src/tests/http_tests.cpp > 800752a57230d7768d3d741fef87f6283757e424 > > Diff: https://reviews.apache.org/r/31930/diff/ > > > Testing > ------- > > * added tests > * make check > > > Thanks, > > Ben Mahler > >
