You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2012/07/10 00:01:01 UTC
Review Request: FLAGS (1): Added a "flags" abstraction.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5854/
-----------------------------------------------------------
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
Re: Review Request: FLAGS (1): Added a "flags" abstraction.
Posted by John Sirois <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5854/#review9090
-----------------------------------------------------------
Ship it!
Ship It!
- John Sirois
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
>
>
Re: Review Request: FLAGS (1): Added a "flags" abstraction.
Posted by John Sirois <jo...@gmail.com>.
> 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.
>
> Benjamin Hindman wrote:
> 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).
Thanks for the explanation - sgtm.
- John
-----------------------------------------------------------
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
>
>
Re: Review Request: FLAGS (1): Added a "flags" abstraction.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> 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
>
>
Re: Review Request: FLAGS (1): Added a "flags" abstraction.
Posted by John Sirois <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5854/#review9008
-----------------------------------------------------------
src/flags/flags.hpp
<https://reviews.apache.org/r/5854/#comment19166>
s/./)./
src/flags/flags.hpp
<https://reviews.apache.org/r/5854/#comment19179>
2 types to allow for implicit conversions? - naively you'd want the same type for the value and the default
src/flags/flags.hpp
<https://reviews.apache.org/r/5854/#comment19181>
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
src/flags/flags.hpp
<https://reviews.apache.org/r/5854/#comment19182>
Can't you skip the check of Flags1 since FlagsBase already does an internal dup check?
src/flags/flags.hpp
<https://reviews.apache.org/r/5854/#comment19178>
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.
src/flags/flags.hpp
<https://reviews.apache.org/r/5854/#comment19177>
kill trailing ws
src/flags/flags.hpp
<https://reviews.apache.org/r/5854/#comment19176>
*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.
src/tests/flags_tests.cpp
<https://reviews.apache.org/r/5854/#comment19183>
It would be nice to be able to test dup handling with a pluggable abort() strategy - this would address a TODO as well.
- John Sirois
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
>
>