You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Joris Van Remoortere <jo...@mesosphere.io> on 2015/03/30 19:49:01 UTC
[Style Proposal] Disallow Capture by Constant Reference to temporary
I would like to propose we disallow capturing temporaries using a constant
reference:
const T& val = f();
*The reasons, and examples for this are outlined in the proposal here:*
[Review Board]: https://reviews.apache.org/r/32630/
[original markdown version]:
https://gist.github.com/jmlvanre/8a3de53ae88c2d19b375
Please feel free to comment here on the dev-list, though I also urge you to
comment on the Review Request as putting proposal markdowns on RB is
something new we're trying!
Joris
Re: [Style Proposal] Disallow Capture by Constant Reference to temporary
Posted by Michael Park <mc...@gmail.com>.
>
> Great link, it looks as though many of our Try<T> / Option<T> / Result<T> /
> Future<T> functions that return different results based on parameters /
> runtime conditions (e.g. t, None(), Error("..."), Failure("...")) will not
> have copies elided via NRVO? (these look similar to cases N and O in the
> doc).
Ben, your observation there is accurate. It's a case we either call it
"good enough" or consider it as a special case to remember to explicitly
move them out.
In relation to this particular proposal, these don't kick in even if we try
capturing it by *const-ref*.
Tested with gcc-4.7.4 and gcc-4.8.4.
MPark.
Re: [Style Proposal] Disallow Capture by Constant Reference to temporary
Posted by Benjamin Mahler <be...@gmail.com>.
Great link, it looks as though many of our Try<T> / Option<T> / Result<T> /
Future<T> functions that return different results based on parameters /
runtime conditions (e.g. t, None(), Error("..."), Failure("...")) will not
have copies elided via NRVO? (these look similar to cases N and O in the
doc).
On Mon, Mar 30, 2015 at 3:43 PM, Cody Maloney <co...@mesosphere.io> wrote:
> There is a fairly comprehensive overview of what compilers do / don't do
> for RVO + NRVO at:
>
> http://www.byte-physics.de/index.php?eID=tx_nawsecuredl&u=0&g=0&t=1427845187&hash=d85e8402f1e5b835e3ff4f266a0a7256516ea57e&file=fileadmin/user_upload/kundenbereich/copy-elision-revisited-v1.0.pdf
>
> Move constructors you'll likely still have to copy some of the data (The
> size of the vector for instance), but you save copying anything that is
> pointed in (the data in the vector). Definitely it makes it a lot cheaper
> for a lot of things, and things smaller than 1024 bytes most likely the
> copy is cheaper than any sort of reference scheme on a modern processor.
>
>
>
>
> On Mon, Mar 30, 2015 at 3:31 PM, Benjamin Mahler <
> benjamin.mahler@gmail.com>
> wrote:
>
> > IIRC, it was when we changed Option:::get to return a const ref. Some
> > callers of 'Option<V> hashmap<K,V>::get()' were grabbing a const ref to
> the
> > temporary coming from the option (case 1 in your markdown), and it then
> > became a temporary to a temporary's member (case 2 in your markdown):
> >
> > E.g.: https://reviews.apache.org/r/18386/diff/#6
> >
> > Re: optimizations
> >
> > Do you know what the state of the art is for RVO and NRVO? When is the
> > compiler unable to perform these optimizations?
> > I noticed you didn't mention move constructors. It looks like move
> > constructors guarantee we elide the copy into the caller's object, which
> > seems to be the whole motivation behind capturing a const ref?
> >
> > On Mon, Mar 30, 2015 at 12:15 PM, Jie Yu <jy...@twitter.com.invalid>
> wrote:
> >
> > > I am a +1 on this too. We had a similar bug long time ago when we
> changed
> > > the return value of Try::get() to be const ref.
> > >
> > > On Mon, Mar 30, 2015 at 11:52 AM, Niklas Nielsen <niklas@mesosphere.io
> >
> > > wrote:
> > >
> > > > Big +1; You could also mention the recent bug in the Two fold
> > > > authentication patch that you guys discovered with this bug.
> > > >
> > > > Niklas
> > > >
> > > > On 30 March 2015 at 10:49, Joris Van Remoortere <joris@mesosphere.io
> >
> > > > wrote:
> > > >
> > > > > I would like to propose we disallow capturing temporaries using a
> > > > constant
> > > > > reference:
> > > > > const T& val = f();
> > > > >
> > > > > *The reasons, and examples for this are outlined in the proposal
> > here:*
> > > > > [Review Board]: https://reviews.apache.org/r/32630/
> > > > > [original markdown version]:
> > > > > https://gist.github.com/jmlvanre/8a3de53ae88c2d19b375
> > > > >
> > > > >
> > > > > Please feel free to comment here on the dev-list, though I also
> urge
> > > you
> > > > to
> > > > > comment on the Review Request as putting proposal markdowns on RB
> is
> > > > > something new we're trying!
> > > > >
> > > > > Joris
> > > > >
> > > >
> > >
> >
>
Re: [Style Proposal] Disallow Capture by Constant Reference to temporary
Posted by Cody Maloney <co...@mesosphere.io>.
There is a fairly comprehensive overview of what compilers do / don't do
for RVO + NRVO at:
http://www.byte-physics.de/index.php?eID=tx_nawsecuredl&u=0&g=0&t=1427845187&hash=d85e8402f1e5b835e3ff4f266a0a7256516ea57e&file=fileadmin/user_upload/kundenbereich/copy-elision-revisited-v1.0.pdf
Move constructors you'll likely still have to copy some of the data (The
size of the vector for instance), but you save copying anything that is
pointed in (the data in the vector). Definitely it makes it a lot cheaper
for a lot of things, and things smaller than 1024 bytes most likely the
copy is cheaper than any sort of reference scheme on a modern processor.
On Mon, Mar 30, 2015 at 3:31 PM, Benjamin Mahler <be...@gmail.com>
wrote:
> IIRC, it was when we changed Option:::get to return a const ref. Some
> callers of 'Option<V> hashmap<K,V>::get()' were grabbing a const ref to the
> temporary coming from the option (case 1 in your markdown), and it then
> became a temporary to a temporary's member (case 2 in your markdown):
>
> E.g.: https://reviews.apache.org/r/18386/diff/#6
>
> Re: optimizations
>
> Do you know what the state of the art is for RVO and NRVO? When is the
> compiler unable to perform these optimizations?
> I noticed you didn't mention move constructors. It looks like move
> constructors guarantee we elide the copy into the caller's object, which
> seems to be the whole motivation behind capturing a const ref?
>
> On Mon, Mar 30, 2015 at 12:15 PM, Jie Yu <jy...@twitter.com.invalid> wrote:
>
> > I am a +1 on this too. We had a similar bug long time ago when we changed
> > the return value of Try::get() to be const ref.
> >
> > On Mon, Mar 30, 2015 at 11:52 AM, Niklas Nielsen <ni...@mesosphere.io>
> > wrote:
> >
> > > Big +1; You could also mention the recent bug in the Two fold
> > > authentication patch that you guys discovered with this bug.
> > >
> > > Niklas
> > >
> > > On 30 March 2015 at 10:49, Joris Van Remoortere <jo...@mesosphere.io>
> > > wrote:
> > >
> > > > I would like to propose we disallow capturing temporaries using a
> > > constant
> > > > reference:
> > > > const T& val = f();
> > > >
> > > > *The reasons, and examples for this are outlined in the proposal
> here:*
> > > > [Review Board]: https://reviews.apache.org/r/32630/
> > > > [original markdown version]:
> > > > https://gist.github.com/jmlvanre/8a3de53ae88c2d19b375
> > > >
> > > >
> > > > Please feel free to comment here on the dev-list, though I also urge
> > you
> > > to
> > > > comment on the Review Request as putting proposal markdowns on RB is
> > > > something new we're trying!
> > > >
> > > > Joris
> > > >
> > >
> >
>
Re: [Style Proposal] Disallow Capture by Constant Reference to temporary
Posted by Benjamin Mahler <be...@gmail.com>.
IIRC, it was when we changed Option:::get to return a const ref. Some
callers of 'Option<V> hashmap<K,V>::get()' were grabbing a const ref to the
temporary coming from the option (case 1 in your markdown), and it then
became a temporary to a temporary's member (case 2 in your markdown):
E.g.: https://reviews.apache.org/r/18386/diff/#6
Re: optimizations
Do you know what the state of the art is for RVO and NRVO? When is the
compiler unable to perform these optimizations?
I noticed you didn't mention move constructors. It looks like move
constructors guarantee we elide the copy into the caller's object, which
seems to be the whole motivation behind capturing a const ref?
On Mon, Mar 30, 2015 at 12:15 PM, Jie Yu <jy...@twitter.com.invalid> wrote:
> I am a +1 on this too. We had a similar bug long time ago when we changed
> the return value of Try::get() to be const ref.
>
> On Mon, Mar 30, 2015 at 11:52 AM, Niklas Nielsen <ni...@mesosphere.io>
> wrote:
>
> > Big +1; You could also mention the recent bug in the Two fold
> > authentication patch that you guys discovered with this bug.
> >
> > Niklas
> >
> > On 30 March 2015 at 10:49, Joris Van Remoortere <jo...@mesosphere.io>
> > wrote:
> >
> > > I would like to propose we disallow capturing temporaries using a
> > constant
> > > reference:
> > > const T& val = f();
> > >
> > > *The reasons, and examples for this are outlined in the proposal here:*
> > > [Review Board]: https://reviews.apache.org/r/32630/
> > > [original markdown version]:
> > > https://gist.github.com/jmlvanre/8a3de53ae88c2d19b375
> > >
> > >
> > > Please feel free to comment here on the dev-list, though I also urge
> you
> > to
> > > comment on the Review Request as putting proposal markdowns on RB is
> > > something new we're trying!
> > >
> > > Joris
> > >
> >
>
Re: [Style Proposal] Disallow Capture by Constant Reference to temporary
Posted by Jie Yu <jy...@twitter.com.INVALID>.
I am a +1 on this too. We had a similar bug long time ago when we changed
the return value of Try::get() to be const ref.
On Mon, Mar 30, 2015 at 11:52 AM, Niklas Nielsen <ni...@mesosphere.io>
wrote:
> Big +1; You could also mention the recent bug in the Two fold
> authentication patch that you guys discovered with this bug.
>
> Niklas
>
> On 30 March 2015 at 10:49, Joris Van Remoortere <jo...@mesosphere.io>
> wrote:
>
> > I would like to propose we disallow capturing temporaries using a
> constant
> > reference:
> > const T& val = f();
> >
> > *The reasons, and examples for this are outlined in the proposal here:*
> > [Review Board]: https://reviews.apache.org/r/32630/
> > [original markdown version]:
> > https://gist.github.com/jmlvanre/8a3de53ae88c2d19b375
> >
> >
> > Please feel free to comment here on the dev-list, though I also urge you
> to
> > comment on the Review Request as putting proposal markdowns on RB is
> > something new we're trying!
> >
> > Joris
> >
>
Re: [Style Proposal] Disallow Capture by Constant Reference to temporary
Posted by Niklas Nielsen <ni...@mesosphere.io>.
Big +1; You could also mention the recent bug in the Two fold
authentication patch that you guys discovered with this bug.
Niklas
On 30 March 2015 at 10:49, Joris Van Remoortere <jo...@mesosphere.io> wrote:
> I would like to propose we disallow capturing temporaries using a constant
> reference:
> const T& val = f();
>
> *The reasons, and examples for this are outlined in the proposal here:*
> [Review Board]: https://reviews.apache.org/r/32630/
> [original markdown version]:
> https://gist.github.com/jmlvanre/8a3de53ae88c2d19b375
>
>
> Please feel free to comment here on the dev-list, though I also urge you to
> comment on the Review Request as putting proposal markdowns on RB is
> something new we're trying!
>
> Joris
>