----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22764/#review46596 -----------------------------------------------------------
Make sure to split apart this change across mesos / libprocess / stout. At first I thought that this might belong more in mesos than in stout, but I see the utility of this now that I understand it better. I think the documentation could describe more about what this is to be used for rather than how it is to be used. I can take this from benh if he does not have time to review this. 3rdparty/libprocess/3rdparty/stout/include/stout/subcommand.hpp <https://reviews.apache.org/r/22764/#comment82087> Do we want to say subprocess? Seems like the implementation doesn't require that? Also subcommand implies a command, the command in this context is the binary that leverages the subcommand abstraction? I think we need to emphasize more on what this is to be used for. Here's a little blurb that might be useful? " Subcommand is an abstraction for creating command binaries that encompass many subcommands. For example: ./runner start --later ./runner stop --now Here, the 'runner' command contains two Subcommand implementations, e.g. StartCommand and StopCommand. " 3rdparty/libprocess/3rdparty/stout/include/stout/subcommand.hpp <https://reviews.apache.org/r/22764/#comment82086> Instead of requiring an override, could we have a 'name' argument in the Subcommand constructor and use that as a member or a non-virtual 'name' method? 3rdparty/libprocess/3rdparty/stout/include/stout/subcommand.hpp <https://reviews.apache.org/r/22764/#comment82085> Should this just be s/getFlags/flags/ similar to how you named 'name()'? 3rdparty/libprocess/3rdparty/stout/include/stout/subcommand.hpp <https://reviews.apache.org/r/22764/#comment82082> This seems like an odd way for this error to manifest, is this something that the caller should be aware of instead of being printed out to the end user? 3rdparty/libprocess/3rdparty/stout/include/stout/subcommand.hpp <https://reviews.apache.org/r/22764/#comment82081> return 0? 3rdparty/libprocess/3rdparty/stout/include/stout/subcommand.hpp <https://reviews.apache.org/r/22764/#comment82080> Do we want to check for duplicate names? 3rdparty/libprocess/3rdparty/stout/include/stout/subcommand.hpp <https://reviews.apache.org/r/22764/#comment82084> Don't you need to shift the argv array before you pass it for flag loading? Seems like you don't want to pass the 'subcommand' part of argv. - Ben Mahler On June 21, 2014, 4:53 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22764/ > ----------------------------------------------------------- > > (Updated June 21, 2014, 4:53 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 fd2c80f > 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 > >
