mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Hindman" <b...@berkeley.edu>
Subject Re: Review Request 34193: Refactored common functionality into BaseFlags
Date Sun, 24 May 2015 18:35:26 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message