-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34193/#review85098
-----------------------------------------------------------

Ship it!


Everything looks good but let's please resolve the issue with 'printUsage' and 
then I'll commit.


3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
<https://reviews.apache.org/r/34193/#comment136570>

    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!).



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
<https://reviews.apache.org/r/34193/#comment136572>

    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?



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
<https://reviews.apache.org/r/34193/#comment136574>

    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].



3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp
<https://reviews.apache.org/r/34193/#comment136575>

    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!



3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp
<https://reviews.apache.org/r/34193/#comment136576>

    With the revised 'usage()' this will just be:
    
    out << flags.usage("This is: TestFlags [options]");


- Benjamin Hindman


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
> 
>

Reply via email to