You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Kamil Domanski <kd...@kdemail.net> on 2014/09/17 16:24:51 UTC
Review Request 25735: change const pass-by-value to const reference
in stout
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25735/
-----------------------------------------------------------
Review request for mesos.
Bugs: MESOS-1805
https://issues.apache.org/jira/browse/MESOS-1805
Repository: mesos-git
Description
-------
os::shell and an overload of strings::internal::fmt in stout pass a const string parameter by copy instead of reference
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/format.hpp 58ab742
3rdparty/libprocess/3rdparty/stout/include/stout/os/shell.hpp 6728ad8
Diff: https://reviews.apache.org/r/25735/diff/
Testing
-------
cd 3rdparty/libprocess/3rdparty/ && make check
Thanks,
Kamil Domanski
Re: Review Request 25735: change const pass-by-value to const
reference in stout
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Oct. 29, 2014, 3:57 p.m., Michael Park wrote:
> > We can't make these `const-ref` unfortunately because the second argument of `va_start` cannot be a reference type.
> >
> > N3797: 18.10 Other runtime support [support.runtime]
> >
> > The restrictions that ISO C places on the second parameter to the va_start() macro in header <stdarg.h>
> > are different in this International Standard. The parameter parmN is the identifier of the rightmost parameter
> > in the variable parameter list of the function definition (the one just before the ...). __If the parameter
> > parmN is of a reference type, or of a type that is not compatible with the type that results when passing an
> > argument for which there is no parameter, the behavior is undefined.__
@Kamil - What's the status here? Sounds like the patch is problematic from mpark's comment. Still want this to go in, or should we drop this and https://issues.apache.org/jira/browse/MESOS-1805 ?
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25735/#review59068
-----------------------------------------------------------
On Sept. 17, 2014, 7:25 a.m., Kamil Domanski wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25735/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 7:25 a.m.)
>
>
> Review request for mesos.
>
>
> Bugs: MESOS-1805
> https://issues.apache.org/jira/browse/MESOS-1805
>
>
> Repository: mesos
>
>
> Description
> -------
>
> os::shell and an overload of strings::internal::fmt in stout pass a const string parameter by copy instead of reference. This patch fixes that.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/format.hpp 58ab742
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/shell.hpp 6728ad8
>
> Diff: https://reviews.apache.org/r/25735/diff/
>
>
> Testing
> -------
>
> cd 3rdparty/libprocess/3rdparty/ && make check
>
>
> Thanks,
>
> Kamil Domanski
>
>
Re: Review Request 25735: change const pass-by-value to const
reference in stout
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25735/#review59068
-----------------------------------------------------------
We can't make these `const-ref` unfortunately because the second argument of `va_start` cannot be a reference type.
N3797: 18.10 Other runtime support [support.runtime]
The restrictions that ISO C places on the second parameter to the va_start() macro in header <stdarg.h>
are different in this International Standard. The parameter parmN is the identifier of the rightmost parameter
in the variable parameter list of the function definition (the one just before the ...). __If the parameter
parmN is of a reference type, or of a type that is not compatible with the type that results when passing an
argument for which there is no parameter, the behavior is undefined.__
- Michael Park
On Sept. 17, 2014, 2:25 p.m., Kamil Domanski wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25735/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 2:25 p.m.)
>
>
> Review request for mesos.
>
>
> Bugs: MESOS-1805
> https://issues.apache.org/jira/browse/MESOS-1805
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> os::shell and an overload of strings::internal::fmt in stout pass a const string parameter by copy instead of reference. This patch fixes that.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/format.hpp 58ab742
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/shell.hpp 6728ad8
>
> Diff: https://reviews.apache.org/r/25735/diff/
>
>
> Testing
> -------
>
> cd 3rdparty/libprocess/3rdparty/ && make check
>
>
> Thanks,
>
> Kamil Domanski
>
>
Re: Review Request 25735: change const pass-by-value to const
reference in stout
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25735/#review53704
-----------------------------------------------------------
Patch looks great!
Reviews applied: [25735]
All tests passed.
- Mesos ReviewBot
On Sept. 17, 2014, 2:25 p.m., Kamil Domanski wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25735/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 2:25 p.m.)
>
>
> Review request for mesos.
>
>
> Bugs: MESOS-1805
> https://issues.apache.org/jira/browse/MESOS-1805
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> os::shell and an overload of strings::internal::fmt in stout pass a const string parameter by copy instead of reference. This patch fixes that.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/format.hpp 58ab742
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/shell.hpp 6728ad8
>
> Diff: https://reviews.apache.org/r/25735/diff/
>
>
> Testing
> -------
>
> cd 3rdparty/libprocess/3rdparty/ && make check
>
>
> Thanks,
>
> Kamil Domanski
>
>
Re: Review Request 25735: change const pass-by-value to const
reference in stout
Posted by Dominic Hamon <dh...@twopensource.com>.
> On Sept. 17, 2014, 9:32 a.m., Dominic Hamon wrote:
> > I'm not convinced that this is necessarily the way we want to go, given the benefits of move semantics in modern compilers, but for consistency we can ship this until we have proof that performance favours the alternative.
>
> Kamil Domanski wrote:
> Isn't move construction more expensive than const reference?
http://stackoverflow.com/questions/21605579/how-true-is-want-speed-pass-by-value
copy, yes. move, no. maybe. ;)
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25735/#review53688
-----------------------------------------------------------
On Sept. 17, 2014, 7:25 a.m., Kamil Domanski wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25735/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 7:25 a.m.)
>
>
> Review request for mesos.
>
>
> Bugs: MESOS-1805
> https://issues.apache.org/jira/browse/MESOS-1805
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> os::shell and an overload of strings::internal::fmt in stout pass a const string parameter by copy instead of reference. This patch fixes that.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/format.hpp 58ab742
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/shell.hpp 6728ad8
>
> Diff: https://reviews.apache.org/r/25735/diff/
>
>
> Testing
> -------
>
> cd 3rdparty/libprocess/3rdparty/ && make check
>
>
> Thanks,
>
> Kamil Domanski
>
>
Re: Review Request 25735: change const pass-by-value to const
reference in stout
Posted by Cody Maloney <co...@mesosphere.io>.
> On Sept. 17, 2014, 4:32 p.m., Dominic Hamon wrote:
> > I'm not convinced that this is necessarily the way we want to go, given the benefits of move semantics in modern compilers, but for consistency we can ship this until we have proof that performance favours the alternative.
>
> Kamil Domanski wrote:
> Isn't move construction more expensive than const reference?
>
> Dominic Hamon wrote:
> http://stackoverflow.com/questions/21605579/how-true-is-want-speed-pass-by-value
>
> copy, yes. move, no. maybe. ;)
http://www.youtube.com/watch?v=xnqTKD8uD64#t=3145 - the current guidelines. Passing by value probably isn't what you want. If you want to get the extra win for r-value referenes you should add another overload, or accept via perfect forwarding.
Also note GCC libstdc++ doesn't implement the C++11/C++14 std::string, so it is Copy on Write (C++89) which means that copies are a little cheaper than they would be otherwise.
- Cody
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25735/#review53688
-----------------------------------------------------------
On Sept. 17, 2014, 2:25 p.m., Kamil Domanski wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25735/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 2:25 p.m.)
>
>
> Review request for mesos.
>
>
> Bugs: MESOS-1805
> https://issues.apache.org/jira/browse/MESOS-1805
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> os::shell and an overload of strings::internal::fmt in stout pass a const string parameter by copy instead of reference. This patch fixes that.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/format.hpp 58ab742
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/shell.hpp 6728ad8
>
> Diff: https://reviews.apache.org/r/25735/diff/
>
>
> Testing
> -------
>
> cd 3rdparty/libprocess/3rdparty/ && make check
>
>
> Thanks,
>
> Kamil Domanski
>
>
Re: Review Request 25735: change const pass-by-value to const
reference in stout
Posted by Kamil Domanski <kd...@kdemail.net>.
> On Sept. 17, 2014, 6:32 p.m., Dominic Hamon wrote:
> > I'm not convinced that this is necessarily the way we want to go, given the benefits of move semantics in modern compilers, but for consistency we can ship this until we have proof that performance favours the alternative.
Isn't move construction more expensive than const reference?
- Kamil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25735/#review53688
-----------------------------------------------------------
On Sept. 17, 2014, 4:25 p.m., Kamil Domanski wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25735/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 4:25 p.m.)
>
>
> Review request for mesos.
>
>
> Bugs: MESOS-1805
> https://issues.apache.org/jira/browse/MESOS-1805
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> os::shell and an overload of strings::internal::fmt in stout pass a const string parameter by copy instead of reference. This patch fixes that.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/format.hpp 58ab742
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/shell.hpp 6728ad8
>
> Diff: https://reviews.apache.org/r/25735/diff/
>
>
> Testing
> -------
>
> cd 3rdparty/libprocess/3rdparty/ && make check
>
>
> Thanks,
>
> Kamil Domanski
>
>
Re: Review Request 25735: change const pass-by-value to const
reference in stout
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25735/#review53688
-----------------------------------------------------------
Ship it!
I'm not convinced that this is necessarily the way we want to go, given the benefits of move semantics in modern compilers, but for consistency we can ship this until we have proof that performance favours the alternative.
- Dominic Hamon
On Sept. 17, 2014, 7:25 a.m., Kamil Domanski wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25735/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 7:25 a.m.)
>
>
> Review request for mesos.
>
>
> Bugs: MESOS-1805
> https://issues.apache.org/jira/browse/MESOS-1805
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> os::shell and an overload of strings::internal::fmt in stout pass a const string parameter by copy instead of reference. This patch fixes that.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/format.hpp 58ab742
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/shell.hpp 6728ad8
>
> Diff: https://reviews.apache.org/r/25735/diff/
>
>
> Testing
> -------
>
> cd 3rdparty/libprocess/3rdparty/ && make check
>
>
> Thanks,
>
> Kamil Domanski
>
>
Re: Review Request 25735: change const pass-by-value to const
reference in stout
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25735/#review57923
-----------------------------------------------------------
Ship it!
LGTM modulo running "make check" on mesos.
@Kamil: Can you rebase and run make check on the top level of the repo?
- Vinod Kone
On Sept. 17, 2014, 2:25 p.m., Kamil Domanski wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25735/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 2:25 p.m.)
>
>
> Review request for mesos.
>
>
> Bugs: MESOS-1805
> https://issues.apache.org/jira/browse/MESOS-1805
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> os::shell and an overload of strings::internal::fmt in stout pass a const string parameter by copy instead of reference. This patch fixes that.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/format.hpp 58ab742
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/shell.hpp 6728ad8
>
> Diff: https://reviews.apache.org/r/25735/diff/
>
>
> Testing
> -------
>
> cd 3rdparty/libprocess/3rdparty/ && make check
>
>
> Thanks,
>
> Kamil Domanski
>
>
Re: Review Request 25735: change const pass-by-value to const
reference in stout
Posted by Kamil Domanski <kd...@kdemail.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25735/
-----------------------------------------------------------
(Updated Sept. 17, 2014, 4:25 p.m.)
Review request for mesos.
Bugs: MESOS-1805
https://issues.apache.org/jira/browse/MESOS-1805
Repository: mesos-git
Description
-------
os::shell and an overload of strings::internal::fmt in stout pass a const string parameter by copy instead of reference. This patch fixes that.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/format.hpp 58ab742
3rdparty/libprocess/3rdparty/stout/include/stout/os/shell.hpp 6728ad8
Diff: https://reviews.apache.org/r/25735/diff/
Testing
-------
cd 3rdparty/libprocess/3rdparty/ && make check
Thanks,
Kamil Domanski
Re: Review Request 25735: change const pass-by-value to const
reference in stout
Posted by Kamil Domanski <kd...@kdemail.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25735/
-----------------------------------------------------------
(Updated Sept. 17, 2014, 4:25 p.m.)
Review request for mesos.
Bugs: MESOS-1805
https://issues.apache.org/jira/browse/MESOS-1805
Repository: mesos-git
Description (updated)
-------
os::shell and an overload of strings::internal::fmt in stout pass a const string parameter by copy instead of reference. This patch fixes that.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/format.hpp 58ab742
3rdparty/libprocess/3rdparty/stout/include/stout/os/shell.hpp 6728ad8
Diff: https://reviews.apache.org/r/25735/diff/
Testing
-------
cd 3rdparty/libprocess/3rdparty/ && make check
Thanks,
Kamil Domanski