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 34943: Added validation to flags.
Date Fri, 05 Jun 2015 13:25:04 GMT


> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 141
> > <https://reviews.apache.org/r/34943/diff/1/?file=976658#file976658line141>
> >
> >     can you please add some documentation to explain what each type (and the relative
@param) is?
> >     (using Doxygen would be awesome :)

Most definitely, but I'll do that in another review.


> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 146
> > <https://reviews.apache.org/r/34943/diff/1/?file=976658#file976658line146>
> >
> >     How do we enforce that F is a function, and not, say, a std::string?

It gets enforced by using 'F'as a function in the body of 'add'. Bottom line, we take any
functor here, if we can invoke the apply operator than we're good to go.


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

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


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

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?


> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, lines 514-517
> > <https://reviews.apache.org/r/34943/diff/1/?file=976659#file976659line514>
> >
> >     I'll admit I'm still a bith hazy about the new { } initializer, but could have
this been:
> >     ```
> >     char* argv[] = { (char*) "/path/to/program",
> >                      (char*) "--name1=billy joel" };
> >     ```

I just copied this code. Let me give this a test and if so I'll cleanup the others too.


- Benjamin


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


On June 2, 2015, 2:43 p.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34943/
> -----------------------------------------------------------
> 
> (Updated June 2, 2015, 2:43 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b

>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 61a405f225d14acbc38a80d35570426cb05a3d0a

>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp a6e8ba943d97ae908122a444332155ebc6c7bb93

> 
> Diff: https://reviews.apache.org/r/34943/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>


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