You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2012/07/31 07:42:03 UTC
Review Request: Fixes operations on Ranges
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6224/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and John Sirois.
Description
-------
See summary.
This addresses bug MESOS-247.
https://issues.apache.org/jira/browse/MESOS-247
Diffs
-----
src/common/values.cpp 0e9dd4f
src/tests/resources_tests.cpp 3e144ed
Diff: https://reviews.apache.org/r/6224/diff/
Testing
-------
make check
Please let me know any other tests you can think of. Lets fix ranges foreva.
Thanks,
Vinod Kone
Re: Review Request: Fixes operations on Ranges
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6224/#review9654
-----------------------------------------------------------
Ship it!
src/common/values.cpp
<https://reviews.apache.org/r/6224/#comment20571>
Put && on previous line.
src/common/values.cpp
<https://reviews.apache.org/r/6224/#comment20573>
why_you_snake_case?
- Benjamin Hindman
On July 31, 2012, 5:08 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6224/
> -----------------------------------------------------------
>
> (Updated July 31, 2012, 5:08 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and John Sirois.
>
>
> Description
> -------
>
> See summary.
>
>
> This addresses bug MESOS-247.
> https://issues.apache.org/jira/browse/MESOS-247
>
>
> Diffs
> -----
>
> src/common/values.cpp 0e9dd4f
> src/tests/resources_tests.cpp 3e144ed
>
> Diff: https://reviews.apache.org/r/6224/diff/
>
>
> Testing
> -------
>
> make check
>
> Please let me know any other tests you can think of. Lets fix ranges foreva.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Fixes operations on Ranges
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6224/
-----------------------------------------------------------
(Updated July 31, 2012, 6:25 p.m.)
Review request for mesos, Benjamin Hindman and John Sirois.
Changes
-------
ben's comments
Description
-------
See summary.
This addresses bug MESOS-247.
https://issues.apache.org/jira/browse/MESOS-247
Diffs (updated)
-----
src/common/values.cpp 0e9dd4f
src/tests/resources_tests.cpp 3e144ed
Diff: https://reviews.apache.org/r/6224/diff/
Testing
-------
make check
Please let me know any other tests you can think of. Lets fix ranges foreva.
Thanks,
Vinod Kone
Re: Review Request: Fixes operations on Ranges
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6224/
-----------------------------------------------------------
(Updated July 31, 2012, 5:08 p.m.)
Review request for mesos, Benjamin Hindman and John Sirois.
Changes
-------
bill's comments
Description
-------
See summary.
This addresses bug MESOS-247.
https://issues.apache.org/jira/browse/MESOS-247
Diffs (updated)
-----
src/common/values.cpp 0e9dd4f
src/tests/resources_tests.cpp 3e144ed
Diff: https://reviews.apache.org/r/6224/diff/
Testing
-------
make check
Please let me know any other tests you can think of. Lets fix ranges foreva.
Thanks,
Vinod Kone
Re: Review Request: Fixes operations on Ranges
Posted by Vinod Kone <vi...@gmail.com>.
> On July 31, 2012, 6:17 a.m., Bill Farner wrote:
> > src/common/values.cpp, line 287
> > <https://reviews.apache.org/r/6224/diff/2/?file=131052#file131052line287>
> >
> > mismatch in indent/wrap style - some places the operator is on the first line, others it's on the continued line
fixed
> On July 31, 2012, 6:17 a.m., Bill Farner wrote:
> > src/common/values.cpp, line 203
> > <https://reviews.apache.org/r/6224/diff/2/?file=131052#file131052line203>
> >
> > Some inner parens would be nice to prevent second-guessing the order of operations.
> >
> > Also, ASCII art would be fantastic - i had to stare at this for a bit to be convinced that it works.
> > Feel free to steal my work below.
> >
> > // b e
> > // current |- - - -|
> > // range |- - - -|
> > // b e
> > // current |- - - -|
> > // range |- - - -|
> > // b e
>
> Bill Farner wrote:
> Err, well it looked nice in the non fixed-width font for the comment editor, you'd have to adjust the spacing for the b/e markers.
added ascii art similar to other functions in this file
> On July 31, 2012, 6:17 a.m., Bill Farner wrote:
> > src/common/values.cpp, line 272
> > <https://reviews.apache.org/r/6224/diff/2/?file=131052#file131052line272>
> >
> > this block is repeated 4 times, a helper function would be nice.
overloaded coalesce() to make this simpler
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6224/#review9625
-----------------------------------------------------------
On July 31, 2012, 5:42 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6224/
> -----------------------------------------------------------
>
> (Updated July 31, 2012, 5:42 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and John Sirois.
>
>
> Description
> -------
>
> See summary.
>
>
> This addresses bug MESOS-247.
> https://issues.apache.org/jira/browse/MESOS-247
>
>
> Diffs
> -----
>
> src/common/values.cpp 0e9dd4f
> src/tests/resources_tests.cpp 3e144ed
>
> Diff: https://reviews.apache.org/r/6224/diff/
>
>
> Testing
> -------
>
> make check
>
> Please let me know any other tests you can think of. Lets fix ranges foreva.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Fixes operations on Ranges
Posted by Bill Farner <bi...@twitter.com>.
> On July 31, 2012, 6:17 a.m., Bill Farner wrote:
> > src/common/values.cpp, line 203
> > <https://reviews.apache.org/r/6224/diff/2/?file=131052#file131052line203>
> >
> > Some inner parens would be nice to prevent second-guessing the order of operations.
> >
> > Also, ASCII art would be fantastic - i had to stare at this for a bit to be convinced that it works.
> > Feel free to steal my work below.
> >
> > // b e
> > // current |- - - -|
> > // range |- - - -|
> > // b e
> > // current |- - - -|
> > // range |- - - -|
> > // b e
Err, well it looked nice in the non fixed-width font for the comment editor, you'd have to adjust the spacing for the b/e markers.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6224/#review9625
-----------------------------------------------------------
On July 31, 2012, 5:42 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6224/
> -----------------------------------------------------------
>
> (Updated July 31, 2012, 5:42 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and John Sirois.
>
>
> Description
> -------
>
> See summary.
>
>
> This addresses bug MESOS-247.
> https://issues.apache.org/jira/browse/MESOS-247
>
>
> Diffs
> -----
>
> src/common/values.cpp 0e9dd4f
> src/tests/resources_tests.cpp 3e144ed
>
> Diff: https://reviews.apache.org/r/6224/diff/
>
>
> Testing
> -------
>
> make check
>
> Please let me know any other tests you can think of. Lets fix ranges foreva.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Fixes operations on Ranges
Posted by Bill Farner <bi...@twitter.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6224/#review9625
-----------------------------------------------------------
Ship it!
src/common/values.cpp
<https://reviews.apache.org/r/6224/#comment20534>
Some inner parens would be nice to prevent second-guessing the order of operations.
Also, ASCII art would be fantastic - i had to stare at this for a bit to be convinced that it works.
Feel free to steal my work below.
// b e
// current |- - - -|
// range |- - - -|
// b e
// current |- - - -|
// range |- - - -|
// b e
src/common/values.cpp
<https://reviews.apache.org/r/6224/#comment20535>
this block is repeated 4 times, a helper function would be nice.
src/common/values.cpp
<https://reviews.apache.org/r/6224/#comment20536>
mismatch in indent/wrap style - some places the operator is on the first line, others it's on the continued line
- Bill Farner
On July 31, 2012, 5:42 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6224/
> -----------------------------------------------------------
>
> (Updated July 31, 2012, 5:42 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and John Sirois.
>
>
> Description
> -------
>
> See summary.
>
>
> This addresses bug MESOS-247.
> https://issues.apache.org/jira/browse/MESOS-247
>
>
> Diffs
> -----
>
> src/common/values.cpp 0e9dd4f
> src/tests/resources_tests.cpp 3e144ed
>
> Diff: https://reviews.apache.org/r/6224/diff/
>
>
> Testing
> -------
>
> make check
>
> Please let me know any other tests you can think of. Lets fix ranges foreva.
>
>
> Thanks,
>
> Vinod Kone
>
>