> On July 10, 2012, 4:18 p.m., John Sirois wrote: > > src/flags/flags.hpp, line 60 > > <https://reviews.apache.org/r/5854/diff/1/?file=120760#file120760line60> > > > > s/./)./
This comment gets changed in the future. ;) > On July 10, 2012, 4:18 p.m., John Sirois wrote: > > src/flags/flags.hpp, line 79 > > <https://reviews.apache.org/r/5854/diff/1/?file=120760#file120760line79> > > > > 2 types to allow for implicit conversions? - naively you'd want the > > same type for the value and the default Yes, two types for implicit conversions. This is particularly useful for constant strings, I didn't want people to have to type 'string("default")'. > On July 10, 2012, 4:18 p.m., John Sirois wrote: > > src/flags/flags.hpp, line 119 > > <https://reviews.apache.org/r/5854/diff/1/?file=120760#file120760line119> > > > > The testcase does not show off the value of this piece and there is no > > doc doing similar. I assume this is to allow for modules deifining a flag > > set in a FlagsBase and then a main aggregating a few modules flags into one > > parse. Docs would be good as would a test case that exercises the > > value-add as well as the dup detection between aggregated FlagsBase's I've added more docs in a subsequent review. As we talked about offline, a duplicate detection test is difficult since my current abort strategy is fatal. > On July 10, 2012, 4:18 p.m., John Sirois wrote: > > src/flags/flags.hpp, line 130 > > <https://reviews.apache.org/r/5854/diff/1/?file=120760#file120760line130> > > > > Can't you skip the check of Flags1 since FlagsBase already does an > > internal dup check? The 'flagsN' stuff is removed in the future, so I'm skipping this comment. > On July 10, 2012, 4:18 p.m., John Sirois wrote: > > src/flags/flags.hpp, line 304 > > <https://reviews.apache.org/r/5854/diff/1/?file=120760#file120760line304> > > > > Couldn't you just specialize add for bool? I had the general > > impression it was a win to not link rtti when you don't have to. We use RTTI in other places (e.g., for dynamic_cast), so I decided it's okay here. In general, doing magical flags stuff, to me, really requires more language support, so it's an okay tradeoff. Also, I can't specialize 'add' for bool because you can't partially specialize functions (and there are versions that have more than just the T template argument). > On July 10, 2012, 4:18 p.m., John Sirois wrote: > > src/flags/flags.hpp, line 315 > > <https://reviews.apache.org/r/5854/diff/1/?file=120760#file120760line315> > > > > kill trailing ws Killed in subsequent review. > On July 10, 2012, 4:18 p.m., John Sirois wrote: > > src/flags/flags.hpp, line 407 > > <https://reviews.apache.org/r/5854/diff/1/?file=120760#file120760line407> > > > > *option may point to an initialized variable in which case you leak > > here - it may be worth a std:cerr to warn of bad library usage. I guess > > you could also delete. We briefly chatted about this, and looking at it more carefully the only thing that leaks here is the std::string (or more generally the T) inside the option. The reason that actually leaks is because the '=' operator does not delete the value. I'll put that in another review (because it is a general issue). It's possible that a flags will be "reloaded", in which case the option may in fact be "some", so I don't want to abort on that case (but I do want the underlying data to be freed). > On July 10, 2012, 4:18 p.m., John Sirois wrote: > > src/tests/flags_tests.cpp, line 34 > > <https://reviews.apache.org/r/5854/diff/1/?file=120761#file120761line34> > > > > It would be nice to be able to test dup handling with a pluggable > > abort() strategy - this would address a TODO as well. Agreed, but one thing at a time. ;) - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5854/#review9008 ----------------------------------------------------------- On July 9, 2012, 10:01 p.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5854/ > ----------------------------------------------------------- > > (Updated July 9, 2012, 10:01 p.m.) > > > Review request for mesos, John Sirois and Vinod Kone. > > > Description > ------- > > The first of 13 reviews related to replacing configuration in Mesos with a > "flags" abstraction. In particular, the flags abstraction: > > (1) Eliminates the need to specify defaults in more than one place. > (2) Eliminates the need to specify types for options in more than one place. > > > Diffs > ----- > > src/Makefile.am 11f6b4c > src/common/strings.hpp fbca257 > src/flags/flags.hpp PRE-CREATION > src/tests/flags_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/5854/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
