----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22764/#review46639 -----------------------------------------------------------
Ship it! Cool stuff. Can you split this out into stout and Mesos commits? Don't worry about creating new reviews for them though. 3rdparty/libprocess/3rdparty/stout/include/stout/subcommand.hpp <https://reviews.apache.org/r/22764/#comment82168> Just a thought: I'm not convinced that you might not sometimes want to see all of "your" argc and argv (where I define "your" as being everything after argv[0] and argv[1]). We've seen this in our code base too, see the mesos-executor. Really what we're saving Subcommand writers is the three lines to do flags.load() and check for an error by not passing them their argc and argv. 3rdparty/libprocess/3rdparty/stout/include/stout/subcommand.hpp <https://reviews.apache.org/r/22764/#comment82148> Let's use _ suffix for either all instance members or none please. 3rdparty/libprocess/src/tests/subprocess_tests.cpp <https://reviews.apache.org/r/22764/#comment82152> We should have an independent Subprocess Flags test and a SubcommandTest so we don't conflate the two. Seems like you could define/declare TestSubcommand::Flags outside of TestSubcommand (as it originally was done) but still use it in TestSubcommand. Moreover, you could define a helper that sets up these Flags that gets used both by the original test and the SubcommandTest so that you don't have to duplicate the code. - Benjamin Hindman On June 25, 2014, 6:37 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22764/ > ----------------------------------------------------------- > > (Updated June 25, 2014, 6:37 a.m.) > > > Review request for mesos and Benjamin Hindman. > > > Bugs: MESOS-1511 > https://issues.apache.org/jira/browse/MESOS-1511 > > > Repository: mesos-git > > > Description > ------- > > See summary. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/Makefile.am eac7ab5 > 3rdparty/libprocess/3rdparty/stout/include/stout/subcommand.hpp > PRE-CREATION > 3rdparty/libprocess/src/tests/subprocess_tests.cpp 7dfa384 > src/Makefile.am 861aad2 > src/launcher/launcher.hpp 35cdc69 > src/launcher/launcher.cpp 872e2e8 > src/launcher/main.cpp b497e98 > src/tests/environment.cpp 21b9d1d > src/tests/launcher_tests.cpp e293cc5 > > Diff: https://reviews.apache.org/r/22764/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
