> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 184
> > <https://reviews.apache.org/r/34943/diff/1/?file=976658#file976658line184>
> >
> > too much choice, IMO - there are (if I counted them right) 8 different
> > `add()` variants to choose from (none of them documented :)
> >
> > It's virtually impossible for folks to figure out which to pick and
> > this leads, I believe, to the current 'code by copying' approach.
> >
> > I would cull it down to no more than 2-3 different options, with some
> > sensible default values.
>
> Benjamin Hindman wrote:
> I completely agree this needs documentation, which I'll take on in a
> different review. But there are really only 4 variants here:
>
> (1) The flag _is_ a class member.
> (A) The flag has a default value (i.e., is _not_ an Option<T>).
> (B) The flag does not have a default value (i.e., _is_ an Option<T>).
>
> (2) The flag is _not_ a class member.
> (A) The flag has a default value (i.e., is _not_ an Option<T>).
> (B) The flag does not have a default value (i.e., _is_ an Option<T>).
>
> All 4 of these options can optionally have a validation function, but
> since we can't capture the default validation function with the existing 4
> functions we have to do the delegating function trick, resulting in 8
> functions. Bottom line, this needs to be documented (and if someone has a
> better suggestion to avoid the delgating function trick I'm all ears).
Why should the callers care if it has a default value, type-wise?
My thinking was, let's always use an `Option<T>` for the flag; a, possibly
`None()`, default value and a `required` bool flag:
```
template<typename T>
void add(Option<T>* option,
const std::string& name,
const std::string& help,
const Option<T>& const default = None(),
bool required = false)
```
and similarly for the class member.
So, all we have are *two* options, and no doubt as to which one to use.
I'm sure I'm missing a ton here, though... especially given that:
```
Option<string> foo;
Option<string> bar;
Option<string> def_foo{"default-foo-val"};
flags.add(&foo, "foo", "this is foo!", def_foo, true);
flags.add1(&bar, "bar", "this is BAR");
```
works as intended, but:
```
flags.add(&foo, "foo", "this is foo!", "default-foo-val", true);
```
causes a compile error (even though, I can totally see an `Option(const T& _t)`
constructor that should "just work" (as it very much does for `def_foo`).
> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, lines 500-504
> > <https://reviews.apache.org/r/34943/diff/1/?file=976659#file976659line500>
> >
> > so, with your approach, everyone, every time that they add a required
> > flag, will have to, essentially, copy & paste this code.
> >
> > what I had envisioned, was a simpler way (eg, by adding a `bool
> > required = false` arg) that, if true would automatically do this validation
> > step in `FlagsBase`
> >
> > (of course, we could add another `add()` method that has that
> > signature, that just internally implements this and calls one of the other
> > `add()` ones - making it the ninth option :) )
>
> Benjamin Hindman wrote:
> I wasn't trying to solve the "required" flag problem, I was just trying
> to show an example of a validation function. But I violently agree that this
> is therefore a horribly bad example that sets a bad precedent, so I've
> replaced this with a different validation function.
>
> I also agree that we should introduce a simplier way, such as a bool as
> you suggested (or better an an enumeration). This doesn't completley solve
> the problem, however, since a flag without a default value will still be an
> Option and still require one to do 'CHECK_SOME(flags.flag)'. Got any other
> ideas?
> horribly bad example
I never said that :)
As mentioned above, a flag should *always* be an `Option<T>` with the
difference that, if we had marked it as `required` then we'd be confident that,
once loaded, it does have a value (so, no need to `CHECK_SOME()`) or it would
have already failed.
For the optional (aka `non-required`) ones (without a default value) obviously
they could be `None()` but that is an "expected outcome" and the caller can
take whatever action they deem appropriate.
>From the 'client' perspective:
1. I told you it was `required` --> there must be a value (or you'd have failed
before getting back to me)
2. I gave you a default value --> there must be a value (possibly the default
one)
3. It's neither required, nor has a default --> its absence (`isNone() ==
true`) has a meaning to me and I'll take appropriate action
4. It's required, and I gave you a default value --> I'm bad at logical
thinking ;-)
I would also propose (similarly to the --help case) to add an
`onMissingRequired()` "hook" with the default implementation
```
void onMissingRequired(const string& name)
{
exit(EXIT_FAILURE) << usage("Missing required flag --" + name);
}
```
so that the caller can also take some alternative action.
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/#review86225
-----------------------------------------------------------
On June 5, 2015, 2:48 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34943/
> -----------------------------------------------------------
>
> (Updated June 5, 2015, 2:48 p.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Also refactored existing 'lambda::bind' arguments to use C++11 lambdas,
> enabling us to get rid of our "loader" and "stringifier" functors.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/Makefile.am
> 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp
> 87606d860dea3235ec70d127d3379065d42dc90b
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
> ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp
> 51d3ab020bd2bae1f12b66cac0da5383c813d5d7
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp
> fda5ae1328a23a4371475652f921341dce7448d5
> 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp
> 80450185f60c5b273face490e0bb9e695b0cb984
>
> Diff: https://reviews.apache.org/r/34943/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
