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 2013/02/05 06:57:35 UTC
Review Request: Added stout Error and None (w/ tests).
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9304/
-----------------------------------------------------------
Review request for mesos, Vinod Kone and Ben Mahler.
Description
-------
See summary.
Diffs
-----
src/tests/stout_tests.cpp fffef3e42774e9b090a098a219a92e642d303d27
third_party/libprocess/Makefile.am dad1b65c3fdb7dbdad4e7c3d9c241cc4e89c3325
third_party/libprocess/include/stout/error.hpp PRE-CREATION
third_party/libprocess/include/stout/none.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/9304/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request: Added stout Error and None (w/ tests).
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Feb. 5, 2013, 10:38 p.m., Vinod Kone wrote:
> > third_party/libprocess/include/stout/error.hpp, line 15
> > <https://reviews.apache.org/r/9304/diff/1/?file=255490#file255490line15>
> >
> > You shouldn't use "of course" and "probably" (3 times) in the same sentence :) The former indicates you are confident, the latter indicates you are not.
> >
> > s/Of course, performance/Performance/
;)
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9304/#review16142
-----------------------------------------------------------
On Feb. 8, 2013, 5:48 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9304/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2013, 5:48 a.m.)
>
>
> Review request for mesos, Vinod Kone and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/stout_tests.cpp fffef3e42774e9b090a098a219a92e642d303d27
> third_party/libprocess/Makefile.am dad1b65c3fdb7dbdad4e7c3d9c241cc4e89c3325
> third_party/libprocess/include/stout/error.hpp PRE-CREATION
> third_party/libprocess/include/stout/none.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/9304/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Added stout Error and None (w/ tests).
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9304/#review16142
-----------------------------------------------------------
src/tests/stout_tests.cpp
<https://reviews.apache.org/r/9304/#comment34532>
using std::string at the top and just use string everywhere.
src/tests/stout_tests.cpp
<https://reviews.apache.org/r/9304/#comment34535>
Missing an EXPECT here?
EXPECT_TRUE(t.isError());
src/tests/stout_tests.cpp
<https://reviews.apache.org/r/9304/#comment34539>
Missing expectation again?
third_party/libprocess/include/stout/error.hpp
<https://reviews.apache.org/r/9304/#comment34541>
s/implicitely/implicitly/
s/convertable/convertible/
third_party/libprocess/include/stout/error.hpp
<https://reviews.apache.org/r/9304/#comment34543>
You shouldn't use "of course" and "probably" (3 times) in the same sentence :) The former indicates you are confident, the latter indicates you are not.
s/Of course, performance/Performance/
third_party/libprocess/include/stout/error.hpp
<https://reviews.apache.org/r/9304/#comment34544>
same here..i'm confused.
third_party/libprocess/include/stout/none.hpp
<https://reviews.apache.org/r/9304/#comment34546>
s/implicitely/implicitly/
s/covertable/convertible/
third_party/libprocess/include/stout/none.hpp
<https://reviews.apache.org/r/9304/#comment34547>
s/fields/fields,/
- Vinod Kone
On Feb. 5, 2013, 5:57 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9304/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2013, 5:57 a.m.)
>
>
> Review request for mesos, Vinod Kone and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/stout_tests.cpp fffef3e42774e9b090a098a219a92e642d303d27
> third_party/libprocess/Makefile.am dad1b65c3fdb7dbdad4e7c3d9c241cc4e89c3325
> third_party/libprocess/include/stout/error.hpp PRE-CREATION
> third_party/libprocess/include/stout/none.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/9304/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Added stout Error and None (w/ tests).
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Feb. 5, 2013, 7:36 p.m., Ben Mahler wrote:
> > src/tests/stout_tests.cpp, line 10
> > <https://reviews.apache.org/r/9304/diff/1/?file=255488#file255488line10>
> >
> > It's a bit unfortunate that error.hpp and none.hpp have to be included to be used, despite being tied so closely with try.hpp, result.hpp, and option.hpp.
Let's consider adding some aggregating headers like "functional.hpp" which includes all that's necessary.
> On Feb. 5, 2013, 7:36 p.m., Ben Mahler wrote:
> > third_party/libprocess/include/stout/error.hpp, line 31
> > <https://reviews.apache.org/r/9304/diff/1/?file=255490#file255490line31>
> >
> > Can you give an example here?
> > Are the nested cases in your stout_tests?
> >
> > Ditto for none.hpp
Added an example in the comment.
> On Feb. 5, 2013, 7:36 p.m., Ben Mahler wrote:
> > third_party/libprocess/include/stout/error.hpp, line 23
> > <https://reviews.apache.org/r/9304/diff/1/?file=255490#file255490line23>
> >
> > explicit?
Great idea!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9304/#review16132
-----------------------------------------------------------
On Feb. 8, 2013, 5:48 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9304/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2013, 5:48 a.m.)
>
>
> Review request for mesos, Vinod Kone and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/stout_tests.cpp fffef3e42774e9b090a098a219a92e642d303d27
> third_party/libprocess/Makefile.am dad1b65c3fdb7dbdad4e7c3d9c241cc4e89c3325
> third_party/libprocess/include/stout/error.hpp PRE-CREATION
> third_party/libprocess/include/stout/none.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/9304/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Added stout Error and None (w/ tests).
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9304/#review16132
-----------------------------------------------------------
src/tests/stout_tests.cpp
<https://reviews.apache.org/r/9304/#comment34485>
It's a bit unfortunate that error.hpp and none.hpp have to be included to be used, despite being tied so closely with try.hpp, result.hpp, and option.hpp.
src/tests/stout_tests.cpp
<https://reviews.apache.org/r/9304/#comment34483>
No similar tests for Option?
third_party/libprocess/include/stout/error.hpp
<https://reviews.apache.org/r/9304/#comment34484>
Is there a reason for using a line-width of 70 for comments? Are we not universally using 80?
Ditto for none.hpp
third_party/libprocess/include/stout/error.hpp
<https://reviews.apache.org/r/9304/#comment34486>
explicit?
third_party/libprocess/include/stout/error.hpp
<https://reviews.apache.org/r/9304/#comment34487>
Can you give an example here?
Are the nested cases in your stout_tests?
Ditto for none.hpp
- Ben Mahler
On Feb. 5, 2013, 5:57 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9304/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2013, 5:57 a.m.)
>
>
> Review request for mesos, Vinod Kone and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/stout_tests.cpp fffef3e42774e9b090a098a219a92e642d303d27
> third_party/libprocess/Makefile.am dad1b65c3fdb7dbdad4e7c3d9c241cc4e89c3325
> third_party/libprocess/include/stout/error.hpp PRE-CREATION
> third_party/libprocess/include/stout/none.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/9304/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Added stout Error and None (w/ tests).
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9304/#review16758
-----------------------------------------------------------
Ship it!
Ship It!
- Ben Mahler
On Feb. 8, 2013, 5:48 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9304/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2013, 5:48 a.m.)
>
>
> Review request for mesos, Vinod Kone and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/stout_tests.cpp fffef3e42774e9b090a098a219a92e642d303d27
> third_party/libprocess/Makefile.am dad1b65c3fdb7dbdad4e7c3d9c241cc4e89c3325
> third_party/libprocess/include/stout/error.hpp PRE-CREATION
> third_party/libprocess/include/stout/none.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/9304/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Added stout Error and None (w/ tests).
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9304/#review16762
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On Feb. 8, 2013, 5:48 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9304/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2013, 5:48 a.m.)
>
>
> Review request for mesos, Vinod Kone and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/stout_tests.cpp fffef3e42774e9b090a098a219a92e642d303d27
> third_party/libprocess/Makefile.am dad1b65c3fdb7dbdad4e7c3d9c241cc4e89c3325
> third_party/libprocess/include/stout/error.hpp PRE-CREATION
> third_party/libprocess/include/stout/none.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/9304/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Added stout Error and None (w/ tests).
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9304/
-----------------------------------------------------------
(Updated Feb. 8, 2013, 5:48 a.m.)
Review request for mesos, Vinod Kone and Ben Mahler.
Changes
-------
Addressed comments.
Description
-------
See summary.
Diffs (updated)
-----
src/tests/stout_tests.cpp fffef3e42774e9b090a098a219a92e642d303d27
third_party/libprocess/Makefile.am dad1b65c3fdb7dbdad4e7c3d9c241cc4e89c3325
third_party/libprocess/include/stout/error.hpp PRE-CREATION
third_party/libprocess/include/stout/none.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/9304/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman