> On May 24, 2015, 6:35 p.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 151 > > <https://reviews.apache.org/r/34193/diff/4/?file=965887#file965887line151> > > > > I think it would be great if we could consistently use the trailing > > underscore it for any single class, rather than some fields using it and > > some not as this could confuse people when to use it. In this case, any > > reason not to change all the fields or not use the trailing underscore for > > now?
Funny you mention that, as this pained me too. In our current practice, I discovered, we seem to discourage people from fixing this kind of "localized, hence easy while I'm changing this code" issues - and rather punt to an indeterminate future time by "creating a Jira ticket" (in this case the time of doing so, coupled with the cost of the context switch is ridiculously higher than the two keystrokes required in any modern IDE to fix all the private variables' names). I found myself caught between two bad options: 1. risk everyone's ire by executing a fix that was not sanctioned; or 2. perpetuating a violation of the code style. Just say the word, and I'll happily refactor all private variable names (or just remove the trailing underscore from `programName_` - but this will require special dispensation, as it violates the current style guide). Needless to say, I'd be happier with the former. > On May 24, 2015, 6:35 p.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, lines > > 92-103 > > <https://reviews.apache.org/r/34193/diff/4/?file=965887#file965887line92> > > > > Adhering to the Google Style Guide, we do not use non-const reference > > arguments to functions: > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arguments > > > > As the style guide points out, there are exceptions here, most notably > > 'operator <<' which, by no coincidence, is relevant here and perhaps why > > this slipped through? ;-) > > > > Here's my suggestion to resolve this issue: let's add the 'std::string > > message' to 'usage()'. There wasn't anything special about that function > > that didn't include it originally, so I don't see any reason to not add it > > now. Here's how this would look: > > > > std::string usage(const Option<std::string>& message = None()) const; > > > > Now in the body of 'usage' we can add our default message or replace > > with whatever someone else passed in. And then here is how it would look > > like when used: > > > > if (flags.help) { > > std::cerr << flags.usage("This is: TestFlags [options]") << std::endl; > > } > > > > The alternative here would be to take std::ostream as a pointer, but > > where ever we can cleanly prefer the functional style we should as it leads > > to more composable and easier to reason about code (i.e., avoiding > > non-local manipulation of variables, etc). > > > > Moreover, I actually think it yields cleaner code at the call sites > > where we are already using std::cerr directly, for example: > > > > if (flags.master.isNone()) { > > std::cerr << "Missing required option --master" << std::endl; > > flags.printUsage(std::cerr); > > return EXIT_FAILURE; > > } > > > > Would be cleaner to me as: > > > > if (flags.master.isNone()) { > > std::cerr << "Missing required option --master" << std::endl > > << flags.usage() << std::endl; > > return EXIT_FAILURE; > > } > > > > Finally, to cover all the cases here, I could imagine killing > > 'setUsageMsg' and letting folks define their own constants that they always > > pass in, for example: > > > > const string USAGE_MESSAGE = "This is: TestFlags [Options]"; > > > > ... > > > > std::cerr << flags.usage(USAGE_MESSAGE) << std::endl; > > > > > > To be clear I'm not opposed to having a usage message setter, but I'm > > always in favor of having less ways, or ideally one way, of doing the same > > thing. And regardless, s/setUsageMsg/setUsageMessage/ (it looks like Joris > > mentioned this in a previous review, so just following up here, thanks > > guys!). Just to be clear, it didn't "slip through" :) this was by design (`operator <<` is non-const and also I need to return the ostream as a non-const ref). I quite like your suggestion, though - I can easily implement it. Just to be clear, your last example could have actually also been: ``` if (flags.master.isNone()) { std::cerr << flags.usage("Missing required option --master") << std::endl; return EXIT_FAILURE; } ``` am I getting this right? By also adding a `protected: string defaultUsageMessage_` (which would be initialized in the constructor to be equal to `const string FlagsBase::DEFAULT_USAGE_MESSAGE`) derived classes could change just that, and not need to bother at the point of call. > On May 24, 2015, 6:35 p.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 376 > > <https://reviews.apache.org/r/34193/diff/4/?file=965887#file965887line376> > > > > Any reason not to make 'programName_' be an Option rather than setting > > it to the empty string if argv[0] doesn't exist? While I understand in this > > case we'd probably print the same thing, we definitely have more > > information with an Option versus someone passing us an empty string for > > argv[0]. No particular reason - I don't actually think that having an Option improves the semantics; but equally happy either way. It does make the code a bit more convoluted (`if programName_.isNone()` etc. etc.) bottom line: someone gave us absolutely nothing to parse into Flags - they'll get nothing out of it :) > On May 24, 2015, 6:35 p.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, line 510 > > <https://reviews.apache.org/r/34193/diff/4/?file=965888#file965888line510> > > > > Two newlines between all top-level definitions/declarations please! I > > see Joris called this out in a previous review but he just addressed a > > single place instead of all the places. ;-) Thanks guys! my bad, sorry! this seems something that should be easy to automate with our checks? I see some checks happening before commits, but they clearly are probably incomplete? Or is this something that will be fixed by clang-format? A warning systems that only warns 70% of the time, aint' that good, sir :) > On May 24, 2015, 6:35 p.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, lines 516-517 > > <https://reviews.apache.org/r/34193/diff/4/?file=965888#file965888line516> > > > > With the revised 'usage()' this will just be: > > > > out << flags.usage("This is: TestFlags [options]"); yup! :) - Marco ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34193/#review85098 ----------------------------------------------------------- On May 20, 2015, 11:22 p.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34193/ > ----------------------------------------------------------- > > (Updated May 20, 2015, 11:22 p.m.) > > > Review request for mesos, Benjamin Hindman and Joris Van Remoortere. > > > Bugs: MESOS-2711 > https://issues.apache.org/jira/browse/MESOS-2711 > > > Repository: mesos > > > Description > ------- > > Jira: MESOS-2711 > > Every program that uses stout's `BaseFlags` ends up > re-implementing the `printUsage()` function, and adding > a `bool help` (and associated --help flag); this functionality > has now been refactored in the base class and is available everywhere. > > This change attempts to be backward-compatible, so it > does not alter the behavior of the program when --help is > invoked (by, eg, automatically printing usage and exiting) > but leaves up to the caller to check for `flags.help` and then > decide what action to take. > > There is now a default behavior for the "leader" ("Usage: <prog name> > [options]") > but the client API allows to modify that too. > > Note - anywhere I found the use of the `--help` flag the behavior was the > same: > print usage and exit (see also https://reviews.apache.org/r/34195). > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp > fb383b463a99924483634eebf22bf34de318f920 > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp > 00281195b53d2597bdb46e3fe6cd9d46a5e9b1f1 > > Diff: https://reviews.apache.org/r/34193/diff/ > > > Testing > ------- > > make check > > **NOTE** this change by itself breaks the build, because the --help is > redefined everywhere (16 places, as of last count): CL 34195 fixes that and > makes all build/tests pass. > > > Thanks, > > Marco Massenzio > >
