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