You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Niklas Nielsen <ni...@qni.dk> on 2013/11/20 22:26:38 UTC
Re: Review Request 15285: toString() for flag values.
-----------------------------------------------------------
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.
Changes
-------
Opening for https://issues.apache.org/jira/browse/MESOS-824
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
Re: Review Request 15285: stringify() for flag values.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Dec. 4, 2013, 7:55 p.m., Benjamin Hindman wrote:
> > Ship It!
Great - just committed this patch (with help and guidance from Ross).
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15285/#review29747
-----------------------------------------------------------
On Dec. 4, 2013, 8:18 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15285/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2013, 8:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ross Allen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This patch adds a stringify 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 stringify takes in the owning flags
> instance as a FlagsBase pointer: stringify(FlagsBase*).
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp d31c984
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp cfe996e
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 9af2da1
>
> Diff: https://reviews.apache.org/r/15285/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 15285: toString() for flag values.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15285/#review29747
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Hindman
On Dec. 3, 2013, 2:07 a.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15285/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2013, 2:07 a.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 cfe996e
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 9af2da1
>
> Diff: https://reviews.apache.org/r/15285/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 15285: stringify() for flag values.
Posted by Benjamin Mahler <be...@gmail.com>.
Thanks!
On Tue, Dec 10, 2013 at 12:30 PM, Niklas Nielsen <ni...@qni.dk> wrote:
>
>
> > On Dec. 10, 2013, 7:36 p.m., Ben Mahler wrote:
> > > Hey Nik, it appears this broke 'make distcheck' because the
> stringifier file was not added to src/Makefile.am, can you send out a fix?
>
> Shoot - sorry about that. Yes - I'll get that in.
>
>
> - Niklas
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15285/#review30115
> -----------------------------------------------------------
>
>
> On Dec. 4, 2013, 8:18 p.m., Niklas Nielsen wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/15285/
> > -----------------------------------------------------------
> >
> > (Updated Dec. 4, 2013, 8:18 p.m.)
> >
> >
> > Review request for mesos, Benjamin Hindman and Ross Allen.
> >
> >
> > Repository: mesos-git
> >
> >
> > Description
> > -------
> >
> > This patch adds a stringify 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 stringify takes in the owning flags
> > instance as a FlagsBase pointer: stringify(FlagsBase*).
> >
> >
> > Diffs
> > -----
> >
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp d31c984
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
> cfe996e
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp
> PRE-CREATION
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 9af2da1
> >
> > Diff: https://reviews.apache.org/r/15285/diff/
> >
> >
> > Testing
> > -------
> >
> > make check
> >
> >
> > Thanks,
> >
> > Niklas Nielsen
> >
> >
>
>
Re: Review Request 15285: stringify() for flag values.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Dec. 10, 2013, 7:36 p.m., Ben Mahler wrote:
> > Hey Nik, it appears this broke 'make distcheck' because the stringifier file was not added to src/Makefile.am, can you send out a fix?
Shoot - sorry about that. Yes - I'll get that in.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15285/#review30115
-----------------------------------------------------------
On Dec. 4, 2013, 8:18 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15285/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2013, 8:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ross Allen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This patch adds a stringify 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 stringify takes in the owning flags
> instance as a FlagsBase pointer: stringify(FlagsBase*).
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp d31c984
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp cfe996e
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 9af2da1
>
> Diff: https://reviews.apache.org/r/15285/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 15285: stringify() for flag values.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15285/#review30115
-----------------------------------------------------------
Hey Nik, it appears this broke 'make distcheck' because the stringifier file was not added to src/Makefile.am, can you send out a fix?
- Ben Mahler
On Dec. 4, 2013, 8:18 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15285/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2013, 8:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ross Allen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This patch adds a stringify 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 stringify takes in the owning flags
> instance as a FlagsBase pointer: stringify(FlagsBase*).
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp d31c984
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp cfe996e
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 9af2da1
>
> Diff: https://reviews.apache.org/r/15285/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 15285: stringify() for flag values.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15285/
-----------------------------------------------------------
(Updated Dec. 4, 2013, 8:18 p.m.)
Review request for mesos, Benjamin Hindman and Ross Allen.
Changes
-------
Changed description to match new name of methods: toString -> stringify.
Summary (updated)
-----------------
stringify() for flag values.
Repository: mesos-git
Description (updated)
-------
This patch adds a stringify 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 stringify takes in the owning flags
instance as a FlagsBase pointer: stringify(FlagsBase*).
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp d31c984
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp cfe996e
3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp PRE-CREATION
3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 9af2da1
Diff: https://reviews.apache.org/r/15285/diff/
Testing
-------
make check
Thanks,
Niklas Nielsen
Re: Review Request 15285: toString() for flag values.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15285/
-----------------------------------------------------------
(Updated Dec. 3, 2013, 2:07 a.m.)
Review request for mesos, Benjamin Hindman and Ross Allen.
Changes
-------
Test cases and suggestions from Ben H
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 (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp d31c984
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp cfe996e
3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp PRE-CREATION
3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 9af2da1
Diff: https://reviews.apache.org/r/15285/diff/
Testing
-------
make check
Thanks,
Niklas Nielsen
Re: Review Request 15285: toString() for flag values.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15285/
-----------------------------------------------------------
(Updated Nov. 27, 2013, 4 a.m.)
Review request for mesos, Benjamin Hindman and Ross Allen.
Changes
-------
Added stringifier.hpp
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 (updated)
-----
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/stringifier.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/15285/diff/
Testing
-------
make check
Thanks,
Niklas Nielsen
Re: Review Request 15285: toString() for flag values.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15285/
-----------------------------------------------------------
(Updated Nov. 27, 2013, 3:54 a.m.)
Review request for mesos, Benjamin Hindman and Ross Allen.
Changes
-------
Addressed Ben's comments.
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 (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp d31c984
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp fcaa4e4
Diff: https://reviews.apache.org/r/15285/diff/
Testing
-------
make check
Thanks,
Niklas Nielsen
Re: Review Request 15285: toString() for flag values.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Nov. 25, 2013, 2:22 a.m., Benjamin Hindman wrote:
> > I think moving the stringifier stuff to it's own file sounds good to me too (stout/include/stout/flags/stringifier.hpp).
Sounds good! Added stringier.hpp
> On Nov. 25, 2013, 2:22 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp, line 22
> > <https://reviews.apache.org/r/15285/diff/1/?file=380108#file380108line22>
> >
> > s/toString/stringifier/
> >
> > I would prefer s/toString/stringify/ but then we should probably do s/loader/load/ too (feel free)!
How about if I follow up with a separate loader -> load patch?
> On Nov. 25, 2013, 2:22 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp, line 145
> > <https://reviews.apache.org/r/15285/diff/1/?file=380110#file380110line145>
> >
> > 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. ;)
Changed the stringifier methods to what you suggested :)
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15285/#review29355
-----------------------------------------------------------
On Nov. 27, 2013, 3:54 a.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15285/
> -----------------------------------------------------------
>
> (Updated Nov. 27, 2013, 3:54 a.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
>
> Diff: https://reviews.apache.org/r/15285/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 15285: toString() for flag values.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
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
>
>