You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Dominic Hamon <dh...@twopensource.com> on 2014/02/22 00:45:22 UTC
Review Request 18383: Changed Option::get to return const reference to
reduce copying.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18383/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Bugs: MESOS-1008
https://issues.apache.org/jira/browse/MESOS-1008
Repository: mesos-git
Description
-------
See summary
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 3305d1363adfffc5e54ff1617e6e7c3c29f7e200
3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp d99b75aeae10319b574c67beeb6023358cac7aec
Diff: https://reviews.apache.org/r/18383/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 18383: Changed Option::get to return const reference to
reduce copying.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18383/#review35442
-----------------------------------------------------------
Ship it!
Ship It!
- Ben Mahler
On Feb. 25, 2014, 1:28 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18383/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2014, 1:28 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1008
> https://issues.apache.org/jira/browse/MESOS-1008
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 3305d1363adfffc5e54ff1617e6e7c3c29f7e200
>
> Diff: https://reviews.apache.org/r/18383/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 18383: Changed Option::get to return const reference to
reduce copying.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18383/
-----------------------------------------------------------
(Updated Feb. 24, 2014, 5:28 p.m.)
Review request for mesos and Ben Mahler.
Bugs: MESOS-1008
https://issues.apache.org/jira/browse/MESOS-1008
Repository: mesos-git
Description
-------
See summary
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 3305d1363adfffc5e54ff1617e6e7c3c29f7e200
Diff: https://reviews.apache.org/r/18383/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 18383: Changed Option::get to return const reference to
reduce copying.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18383/
-----------------------------------------------------------
(Updated Feb. 24, 2014, 4:15 p.m.)
Review request for mesos and Ben Mahler.
Bugs: MESOS-1008
https://issues.apache.org/jira/browse/MESOS-1008
Repository: mesos-git
Description
-------
See summary
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 3305d1363adfffc5e54ff1617e6e7c3c29f7e200
Diff: https://reviews.apache.org/r/18383/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 18383: Changed Option::get to return const reference to
reduce copying.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18383/
-----------------------------------------------------------
(Updated Feb. 24, 2014, 3:57 p.m.)
Review request for mesos and Ben Mahler.
Bugs: MESOS-1008
https://issues.apache.org/jira/browse/MESOS-1008
Repository: mesos-git
Description
-------
See summary
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 3305d1363adfffc5e54ff1617e6e7c3c29f7e200
3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp d99b75aeae10319b574c67beeb6023358cac7aec
Diff: https://reviews.apache.org/r/18383/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 18383: Changed Option::get to return const reference to
reduce copying.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On Feb. 24, 2014, 3:42 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp, lines 23-24
> > <https://reviews.apache.org/r/18383/diff/2/?file=500730#file500730line23>
> >
> > Why did you need to add these? If the goal was to provide these automatically for those including option.hpp, it might be good to spell that out here.
> >
> > Benh added Some and None, can you check with him as to why he didn't add these includes implicitly?
I just moved them and changed them from <> to "" as they're not system includes. This will help in terms of readability (figuring out which headers are local to the project) and some build time (minimal but it can help the compiler know which paths to search).
> On Feb. 24, 2014, 3:42 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp, lines 100-104
> > <https://reviews.apache.org/r/18383/diff/2/?file=500731#file500731line100>
> >
> > Perhaps we should make this optimization separately? The error case tends to be an exceptional code path, so perhaps we can optimize this independently only if there's a need?
It doesn't require any changes elsewhere so is a trivial change.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18383/#review35333
-----------------------------------------------------------
On Feb. 24, 2014, 3:57 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18383/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2014, 3:57 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1008
> https://issues.apache.org/jira/browse/MESOS-1008
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 3305d1363adfffc5e54ff1617e6e7c3c29f7e200
> 3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp d99b75aeae10319b574c67beeb6023358cac7aec
>
> Diff: https://reviews.apache.org/r/18383/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 18383: Changed Option::get to return const reference to
reduce copying.
Posted by Ben Mahler <be...@gmail.com>.
> On Feb. 24, 2014, 11:42 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp, lines 100-104
> > <https://reviews.apache.org/r/18383/diff/2/?file=500731#file500731line100>
> >
> > Perhaps we should make this optimization separately? The error case tends to be an exceptional code path, so perhaps we can optimize this independently only if there's a need?
>
> Dominic Hamon wrote:
> It doesn't require any changes elsewhere so is a trivial change.
Ok, let's just make it in a separate commit at least.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18383/#review35333
-----------------------------------------------------------
On Feb. 24, 2014, 11:57 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18383/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2014, 11:57 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1008
> https://issues.apache.org/jira/browse/MESOS-1008
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 3305d1363adfffc5e54ff1617e6e7c3c29f7e200
> 3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp d99b75aeae10319b574c67beeb6023358cac7aec
>
> Diff: https://reviews.apache.org/r/18383/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 18383: Changed Option::get to return const reference to
reduce copying.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18383/#review35333
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp
<https://reviews.apache.org/r/18383/#comment65794>
Since we maintain stout as a separate subproject, we try to avoid library dependencies where we can. In this case, we don't want someone using Option to also have to be using google-logging.
You'll notice we still are violating this in some of our old code (like os.hpp), where there are some old-school functions like getenv that do a LOG(FATAL). :(
At one point we were going to add stack trace functionality to stout so that we could do better assertions in places like Option::get(). For now, can you keep it as an assert?
3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp
<https://reviews.apache.org/r/18383/#comment65793>
Why did you need to add these? If the goal was to provide these automatically for those including option.hpp, it might be good to spell that out here.
Benh added Some and None, can you check with him as to why he didn't add these includes implicitly?
3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp
<https://reviews.apache.org/r/18383/#comment65795>
Ditto here.
3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp
<https://reviews.apache.org/r/18383/#comment65796>
Perhaps we should make this optimization separately? The error case tends to be an exceptional code path, so perhaps we can optimize this independently only if there's a need?
- Ben Mahler
On Feb. 22, 2014, 12:17 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18383/
> -----------------------------------------------------------
>
> (Updated Feb. 22, 2014, 12:17 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1008
> https://issues.apache.org/jira/browse/MESOS-1008
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 3305d1363adfffc5e54ff1617e6e7c3c29f7e200
> 3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp d99b75aeae10319b574c67beeb6023358cac7aec
>
> Diff: https://reviews.apache.org/r/18383/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 18383: Changed Option::get to return const reference to
reduce copying.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18383/
-----------------------------------------------------------
(Updated Feb. 21, 2014, 4:17 p.m.)
Review request for mesos and Ben Mahler.
Bugs: MESOS-1008
https://issues.apache.org/jira/browse/MESOS-1008
Repository: mesos-git
Description
-------
See summary
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 3305d1363adfffc5e54ff1617e6e7c3c29f7e200
3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp d99b75aeae10319b574c67beeb6023358cac7aec
Diff: https://reviews.apache.org/r/18383/diff/
Testing
-------
make check
Thanks,
Dominic Hamon