Re: 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/ --- (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
--- 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
Re: 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/#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
> 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
--- 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
> 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
--- 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
> 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
--- 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 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
> 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 > > 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 > >
