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



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

    can you please add some documentation to explain what each type (and the 
relative @param) is?
    (using Doxygen would be awesome :)



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

    How do we enforce that F is a function, and not, say, a std::string?



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

    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.



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

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



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

    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" };
    ```


- Marco Massenzio


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

Reply via email to