> On July 11, 2012, 5:56 p.m., John Sirois wrote: > > src/flags/flag.hpp, line 18 > > <https://reviews.apache.org/r/5863/diff/1/?file=120836#file120836line18> > > > > In isolation this is odd coupling: > > Flag depends on FlagBase, it has no manifest value type here, but a > > value type is implied by the loader (it clearly knows the type it loads). > > It would seem more natural to have > > <typename T> > > struct Flag<T> > > { > > ... > > std::tr1::function<T(const std::string&)> parser; > > } > >
I need Flag to be typeless, thus no templates. > On July 11, 2012, 5:56 p.m., John Sirois wrote: > > src/flags/flags.hpp, line 126 > > <https://reviews.apache.org/r/5863/diff/1/?file=120837#file120837line126> > > > > urg - I suppose this blocks my Flag<T> suggestion :/ Yup. > On July 11, 2012, 5:56 p.m., John Sirois wrote: > > src/flags/flags.hpp, line 310 > > <https://reviews.apache.org/r/5863/diff/1/?file=120837#file120837line310> > > > > It would be nice if there was an overload where I could pass a loader > > reference for those times when I have a 1-off flag type to parse Yeah, passing a function that does the work would be great. Right now you have to overload the Loader/OptionLoader/MemberLoader/MemberOptionLoader. It's possible that would even be the better "top-level" FlagsBase::add (i.e., the other 'add' functions call that one), but since that shouldn't change this interface I'm going to punt that for later. > On July 11, 2012, 5:56 p.m., John Sirois wrote: > > src/flags/flags.hpp, line 392 > > <https://reviews.apache.org/r/5863/diff/1/?file=120837#file120837line392> > > > > kill Done. > On July 11, 2012, 5:56 p.m., John Sirois wrote: > > src/master/webui.hpp, line 49 > > <https://reviews.apache.org/r/5863/diff/1/?file=120847#file120847line49> > > > > kill trailing ws Killed from a previous review comment. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5863/#review9073 ----------------------------------------------------------- On July 9, 2012, 10:22 p.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5863/ > ----------------------------------------------------------- > > (Updated July 9, 2012, 10:22 p.m.) > > > Review request for mesos, John Sirois and Vinod Kone. > > > Description > ------- > > After converting some things over to use the new flags abstraction, I > realized that I didn't provide everything I really wanted, so I refactored it > a bit. This is really the "flags" abstraction review. > > > Diffs > ----- > > src/Makefile.am 11f6b4c > src/configurator/configurator.hpp 8569645 > src/flags/flag.hpp PRE-CREATION > src/flags/flags.hpp PRE-CREATION > src/flags/loader.hpp PRE-CREATION > src/logging/flags.hpp PRE-CREATION > src/logging/logging.hpp PRE-CREATION > src/logging/logging.cpp PRE-CREATION > src/master/flags.hpp PRE-CREATION > src/master/http.cpp 82784fb > src/master/main.cpp bcbe35a > src/master/master.hpp 886f79c > src/master/master.cpp 44a5dc6 > src/master/webui.hpp 660d5e1 > src/master/webui.cpp 56134b7 > src/tests/flags_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/5863/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
