mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marco Massenzio" <ma...@mesosphere.io>
Subject Re: Review Request 34193: Refactored common functionality into BaseFlags
Date Tue, 26 May 2015 03:24:45 GMT


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


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