----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19162/#review37860 -----------------------------------------------------------
Thanks for pulling out the cleanup review! 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/19162/#comment69624> Why did you re-order these? Can you follow the same include style as elsewhere? 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/19162/#comment69625> This normally goes above, but why did you need this? 3rdparty/libprocess/src/subprocess.cpp <https://reviews.apache.org/r/19162/#comment69803> Can you use os::environ here instead? Would also love to leave a TODO to add 'os::environment()' that returns a map so we can clean up Envp by merging the parent and child environment maps. 3rdparty/libprocess/src/subprocess.cpp <https://reviews.apache.org/r/19162/#comment69822> I like this abstraction! 3rdparty/libprocess/src/subprocess.cpp <https://reviews.apache.org/r/19162/#comment69626> Can you remove the std:: qualifiers? 3rdparty/libprocess/src/subprocess.cpp <https://reviews.apache.org/r/19162/#comment69627> What does '// = delete' mean? Do you still need this now that you've added the comment about what you're trying to restrict? 3rdparty/libprocess/src/subprocess.cpp <https://reviews.apache.org/r/19162/#comment69811> We haven't been using '_' suffixes for member variables. 3rdparty/libprocess/src/subprocess.cpp <https://reviews.apache.org/r/19162/#comment69810> Can you kill this one? 3rdparty/libprocess/src/subprocess.cpp <https://reviews.apache.org/r/19162/#comment69817> Period at the end of the comment. Could indexing make this code cleaner? size_t count = 0; while (environ[count++] != NULL); or size_t count = 0; for (count = 0; environ[count] != NULL; ++count); 3rdparty/libprocess/src/subprocess.cpp <https://reviews.apache.org/r/19162/#comment69820> Would be nice to avoid the need for static_cast and sizeof by just using 'new' in Envp, any reason not to clean it up? Ditto for calloc. 3rdparty/libprocess/src/subprocess.cpp <https://reviews.apache.org/r/19162/#comment69821> Could you just index into 'environ' here as well? 3rdparty/libprocess/src/tests/subprocess_tests.cpp <https://reviews.apache.org/r/19162/#comment69823> System C includes go before C++ includes, can you revert the move? 3rdparty/libprocess/src/tests/subprocess_tests.cpp <https://reviews.apache.org/r/19162/#comment69824> Periods at the end of comments, here and below. 3rdparty/libprocess/src/tests/subprocess_tests.cpp <https://reviews.apache.org/r/19162/#comment69825> I thought the point of taking a map was that we didn't need to quote? We should probably test that spaces work without the need for quoting, no? - Ben Mahler On March 20, 2014, 5:52 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19162/ > ----------------------------------------------------------- > > (Updated March 20, 2014, 5:52 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Repository: mesos-git > > > Description > ------- > > See summary > > > Diffs > ----- > > 3rdparty/libprocess/include/process/subprocess.hpp > 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6 > 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION > 3rdparty/libprocess/src/tests/subprocess_tests.cpp > d15d4d159105474117c4ea432b215431209ab539 > > Diff: https://reviews.apache.org/r/19162/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Dominic Hamon > >
