----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15285/#review29355 -----------------------------------------------------------
I think moving the stringifier stuff to it's own file sounds good to me too (stout/include/stout/flags/stringifier.hpp). 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp <https://reviews.apache.org/r/15285/#comment56549> s/toString/stringifier/ I would prefer s/toString/stringify/ but then we should probably do s/loader/load/ too (feel free)! 3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp <https://reviews.apache.org/r/15285/#comment56552> The flag shouldn't actually be NULL. Note that we don't check for this in the loader either. We should probably be checking for this in FlagsBase::add and returning early (and/or aborting/EXITing) such that we know this to be non-null here. 3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp <https://reviews.apache.org/r/15285/#comment56554> Again, not expecting it to be NULL here. 3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp <https://reviews.apache.org/r/15285/#comment56555> How about making the names be consistent with the Loader names? Loader => Stringifier OptionLoader => OptionStringifier MemberLoader => MemberStringifier OptionMemberLoader => OptionMemberStringifier I'm not opposed to changing Loader names but I'd just like them to be consistent. ;) - Benjamin Hindman On Nov. 20, 2013, 9:26 p.m., Niklas Nielsen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15285/ > ----------------------------------------------------------- > > (Updated Nov. 20, 2013, 9:26 p.m.) > > > Review request for mesos, Benjamin Hindman and Ross Allen. > > > Repository: mesos-git > > > Description > ------- > > This patch adds a toString member function to the Flag structure which lets > us iterate and fetch flag values. > During flag initialization, the toString methods are closed over the flag > value and option references. > Flag values can't be closed over up front as flag values may be changed by > user code arbitrarily. > So toString takes in the owning flags instance as a FlagsBase pointer: > toString(FlagsBase*). > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp d31c984 > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp fcaa4e4 > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp e5eaf24 > > Diff: https://reviews.apache.org/r/15285/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Niklas Nielsen > >
