Make deleted constructors take a const parameter in move-only type macro.

Make deleted constructors take a const parameter in move-only type macro.

Committed: https://crrev.com/e6a653183ba2a1c4f24990904dfc78e32c975f5b
Cr-Commit-Position: refs/heads/master@{#325107}

[mgraczyk, 2015-04-06 23:31:12]

mgraczyk@chromium.org changed reviewers:
+ danakj@chromium.org

[mgraczyk, 2015-04-07 01:10:32]

danakj,

Not sure if you were the best person to review this.  Please feel free to
suggest somebody else if want to.

https://codereview.chromium.org/1061073004/diff/1/base/move.h
File base/move.h (right):

https://codereview.chromium.org/1061073004/diff/1/base/move.h#newcode222
base/move.h:222: type(const type&); \
This allows move-only types to be used in STL containers, as long as the
container is never copied.

[danakj, 2015-04-07 01:26:56]

danakj@chromium.org changed reviewers:
+ thakis@chromium.org

[danakj, 2015-04-07 01:26:57]

There should be some unit tests somewhere that fail without the change and pass
with it. (maybe scoped_ptr_unittest.cc?)

This changes what can bind to those functions, right? I have to think about
this/read some ajwong slides.. so i'll get back to you. +thakis too fyi

[mgraczyk, 2015-04-07 01:33:26]

On 2015/04/07 01:26:57, danakj wrote:
> There should be some unit tests somewhere that fail without the change and
pass
> with it. (maybe scoped_ptr_unittest.cc?)
> 
> This changes what can bind to those functions, right? I have to think about
> this/read some ajwong slides.. so i'll get back to you. +thakis too fyi

Shall I write the test?  I would just add use of scoped_ptr in an STL container.

That's correct, rvalues can no longer bind to these parameters.  That is what we
want, because the use of MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03 implies
that the implementing time should be movable and therefore code like

scoped_ptr x(rvalue());
x = rvalue();

should call the move constructor/assignment operator, not the implicitly deleted
copy constructor/assignment operator.

Could you link to the slides you mentioned?  I am curious what they say.

[Nico, 2015-04-07 01:36:47]

https://codereview.chromium.org/1061073004/diff/1/base/move.h
File base/move.h (right):

https://codereview.chromium.org/1061073004/diff/1/base/move.h#newcode222
base/move.h:222: type(const type&); \
On 2015/04/07 01:10:31, mgraczyk wrote:
> This allows move-only types to be used in STL containers, as long as the
> container is never copied.

Only if your STL knows about move-only types, right?

[danakj, 2015-04-07 02:10:01]

On Mon, Apr 6, 2015 at 6:33 PM, <mgraczyk@chromium.org> wrote:

> On 2015/04/07 01:26:57, danakj wrote:
>
>> There should be some unit tests somewhere that fail without the change and
>>
> pass
>
>> with it. (maybe scoped_ptr_unittest.cc?)
>>
>
>  This changes what can bind to those functions, right? I have to think
>> about
>> this/read some ajwong slides.. so i'll get back to you. +thakis too fyi
>>
>
> Shall I write the test?  I would just add use of scoped_ptr in an STL
> container.
>

We can not use move-only type-aware STL containers yet, because we don't
have c++11 standard library support on all platforms yet. See the
"[chromium-dev] C++11 update" thread.

You'd have to a minimal test case that does something equivalent.


>
> That's correct, rvalues can no longer bind to these parameters.  That is
> what we
>

Do you mean to say rvalues /can/ now bind to these? Before they could not.
As non-const refs require an lvalue.

Here's relevant slides:
https://docs.google.com/a/chromium.org/presentation/d/1ak3bPRQhSSBrGiyfuCOkDx...

But move constructors take priority over copy constructors? Can you give an
example how this helps anything? Maybe the unit test would be demonstrative.


> want, because the use of MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03
> implies
> that the implementing time should be movable and therefore code like
>
> scoped_ptr x(rvalue());
> x = rvalue();
>
> should call the move constructor/assignment operator, not the implicitly
> deleted
> copy constructor/assignment operator.
>
> Could you link to the slides you mentioned?  I am curious what they say.
>
> https://codereview.chromium.org/1061073004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mgraczyk, 2015-04-07 04:15:24]

On 2015/04/07 02:10:01, danakj wrote:
> On Mon, Apr 6, 2015 at 6:33 PM, <mailto:mgraczyk@chromium.org> wrote:
> 
> > On 2015/04/07 01:26:57, danakj wrote:
> >
> >> There should be some unit tests somewhere that fail without the change and
> >>
> > pass
> >
> >> with it. (maybe scoped_ptr_unittest.cc?)
> >>
> >
> >  This changes what can bind to those functions, right? I have to think
> >> about
> >> this/read some ajwong slides.. so i'll get back to you. +thakis too fyi
> >>
> >
> > Shall I write the test?  I would just add use of scoped_ptr in an STL
> > container.
> >
> 
> We can not use move-only type-aware STL containers yet, because we don't
> have c++11 standard library support on all platforms yet. See the
> "[chromium-dev] C++11 update" thread.
> 
> You'd have to a minimal test case that does something equivalent.
> 
> 
> >
> > That's correct, rvalues can no longer bind to these parameters.  That is
> > what we
> >
> 
> Do you mean to say rvalues /can/ now bind to these? Before they could not.
> As non-const refs require an lvalue.
> 
> Here's relevant slides:
>
https://docs.google.com/a/chromium.org/presentation/d/1ak3bPRQhSSBrGiyfuCOkDx...
> 
> But move constructors take priority over copy constructors? Can you give an
> example how this helps anything? Maybe the unit test would be demonstrative.
> 
> 
> > want, because the use of MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03
> > implies
> > that the implementing time should be movable and therefore code like
> >
> > scoped_ptr x(rvalue());
> > x = rvalue();
> >
> > should call the move constructor/assignment operator, not the implicitly
> > deleted
> > copy constructor/assignment operator.
> >
> > Could you link to the slides you mentioned?  I am curious what they say.
> >
> > https://codereview.chromium.org/1061073004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

That's right, I misspoke.  I added a minimal test case that demonstrates the
equivalent problem with using STL containers, but does not itself use any STL
containers.  I figured it would be better to add a new test specifically for
move.h than to put a test in scoped_ptr.h, but please let me know if you'd
rather me not add that new file.

The problem is that since the Container class explicitly defaults its copy
constructors (as is often done with STL containers), its members must have
matching declarations of their copy constructors even if those constructors are
not used.  The test builds and runs with the move.h change, but fails to build
without it.

Nice slides, thanks for the link.  The move constructor emulation trick is no
longer required now that we support a sufficient subset of C++11, right?

I actually spent some time replacing all uses of MOVE_ONLY_TYPE_FOR_CPP_03 move
emulation with MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03 and move
constructors. I have that change in codereview.chromium.org/1054433005.  If
you're interested I can send that out for review once a dependent change in
third_party/mojo gets rolled up to Chromium.

[mgraczyk, 2015-04-07 05:32:59]

https://codereview.chromium.org/1061073004/diff/1/base/move.h
File base/move.h (right):

https://codereview.chromium.org/1061073004/diff/1/base/move.h#newcode222
base/move.h:222: type(const type&); \
On 2015/04/07 01:36:47, Nico wrote:
> On 2015/04/07 01:10:31, mgraczyk wrote:
> > This allows move-only types to be used in STL containers, as long as the
> > container is never copied.
> 
> Only if your STL knows about move-only types, right?

It may or may not work after this change if the library does not have move-aware
containers, but it will work in more situations with the change than without
(unless I missed something thinking about this).  Specifically, it will work in
the case implemented by move_unittest.cc, which is new to this CL.

[Nico, 2015-04-07 17:00:08]

On 2015/04/07 05:32:59, mgraczyk wrote:
> https://codereview.chromium.org/1061073004/diff/1/base/move.h
> File base/move.h (right):
> 
> https://codereview.chromium.org/1061073004/diff/1/base/move.h#newcode222
> base/move.h:222: type(const type&); \
> On 2015/04/07 01:36:47, Nico wrote:
> > On 2015/04/07 01:10:31, mgraczyk wrote:
> > > This allows move-only types to be used in STL containers, as long as the
> > > container is never copied.
> > 
> > Only if your STL knows about move-only types, right?
> 
> It may or may not work after this change if the library does not have
move-aware
> containers, but it will work in more situations with the change than without
> (unless I missed something thinking about this).  Specifically, it will work
in
> the case implemented by move_unittest.cc, which is new to this CL.

That's worse than what we have today though, right? Since we don't allow c++11
library features yet, with this CL folks can do something that will only work on
some platforms (noticeably linux, which most devs use) but which will then fail
at trybot time.

The thing in a test requires using move constructors, which aren't generally
allowed (chromium-cpp.appspot.com)

I'm not sure if we should do this change at this point. danakj, thougths?

[mgraczyk, 2015-04-07 17:45:43]

On 2015/04/07 17:00:08, Nico wrote:
> On 2015/04/07 05:32:59, mgraczyk wrote:
> > https://codereview.chromium.org/1061073004/diff/1/base/move.h
> > File base/move.h (right):
> > 
> > https://codereview.chromium.org/1061073004/diff/1/base/move.h#newcode222
> > base/move.h:222: type(const type&); \
> > On 2015/04/07 01:36:47, Nico wrote:
> > > On 2015/04/07 01:10:31, mgraczyk wrote:
> > > > This allows move-only types to be used in STL containers, as long as the
> > > > container is never copied.
> > > 
> > > Only if your STL knows about move-only types, right?
> > 
> > It may or may not work after this change if the library does not have
> move-aware
> > containers, but it will work in more situations with the change than without
> > (unless I missed something thinking about this).  Specifically, it will work
> in
> > the case implemented by move_unittest.cc, which is new to this CL.
> 
> That's worse than what we have today though, right? Since we don't allow c++11
> library features yet, with this CL folks can do something that will only work
on
> some platforms (noticeably linux, which most devs use) but which will then
fail
> at trybot time.
> 
> The thing in a test requires using move constructors, which aren't generally
> allowed (http://chromium-cpp.appspot.com)
> 
> I'm not sure if we should do this change at this point. danakj, thougths?

The client is required to write a move constructor to use the
MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03 macro.  All of the code
currently using move.h includes implementations of move constructors or the
equivalent emulated version.  The change in this CL is in order to affect
similar implementations that are presupposed to have move constructors.

My unit test works on all platforms, not just Linux.  Are you saying that people
will expect STL containers to "just work" the way Container works on platforms
without C++11 libraries or is there a different concern there?


As for non-move-aware STLs, those will likely not have explicitly defaulted copy
constructors, so they will likely be unaffected by this change.

[brucedawson, 2015-04-08 18:58:52]

The lack of 'const' on the copy constructor in move.h is currently a breaking
error for VS 2015 so this proposed change is very timely.

For some reason VS 2015 (currently in release candidate form and being tested
against Chrome) gives this error when returning a scoped_ptr<BlobDataSnapshot>:

d:\src\chromium2\src\storage\browser\blob\blob_storage_context.cc(157): error
C2248:
'scoped_ptr<storage::BlobDataSnapshot,base::DefaultDeleter<T>>::scoped_ptr':
cannot access private member declared in class
'scoped_ptr<storage::BlobDataSnapshot,base::DefaultDeleter<T>>'

This error goes away when const is added.

In other words, +1 to this change.

[brucedawson, 2015-04-08 20:14:27]

Note that the problem with VS 2015 is only with the release candidate (beta
version). Apparently they have fixed this bug in the final release branch.

[danakj, 2015-04-10 17:32:31]

On 2015/04/07 17:45:43, mgraczyk wrote:
> On 2015/04/07 17:00:08, Nico wrote:
> > On 2015/04/07 05:32:59, mgraczyk wrote:
> > > https://codereview.chromium.org/1061073004/diff/1/base/move.h
> > > File base/move.h (right):
> > > 
> > > https://codereview.chromium.org/1061073004/diff/1/base/move.h#newcode222
> > > base/move.h:222: type(const type&); \
> > > On 2015/04/07 01:36:47, Nico wrote:
> > > > On 2015/04/07 01:10:31, mgraczyk wrote:
> > > > > This allows move-only types to be used in STL containers, as long as
the
> > > > > container is never copied.
> > > > 
> > > > Only if your STL knows about move-only types, right?
> > > 
> > > It may or may not work after this change if the library does not have
> > move-aware
> > > containers, but it will work in more situations with the change than
without
> > > (unless I missed something thinking about this).  Specifically, it will
work
> > in
> > > the case implemented by move_unittest.cc, which is new to this CL.
> > 
> > That's worse than what we have today though, right? Since we don't allow
c++11
> > library features yet, with this CL folks can do something that will only
work
> on
> > some platforms (noticeably linux, which most devs use) but which will then
> fail
> > at trybot time.
> > 
> > The thing in a test requires using move constructors, which aren't generally
> > allowed (http://chromium-cpp.appspot.com)
> > 
> > I'm not sure if we should do this change at this point. danakj, thougths?
> 
> The client is required to write a move constructor to use the
> MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03 macro.  All of the code
> currently using move.h includes implementations of move constructors or the
> equivalent emulated version.  The change in this CL is in order to affect
> similar implementations that are presupposed to have move constructors.
> 
> My unit test works on all platforms, not just Linux.  Are you saying that
people
> will expect STL containers to "just work" the way Container works on platforms
> without C++11 libraries or is there a different concern there?
> 
> 
> As for non-move-aware STLs, those will likely not have explicitly defaulted
copy
> constructors, so they will likely be unaffected by this change.

I think Nico's concern is not about breaking non-move-aware STLs, but rather
making move-aware containers work when they won't on all platforms.

While this is true, I'm not sure purposefully breaking them is the right thing
to do. The const T& in this patch is strictly more correct AFAICT and when I
converted scoped_ptr to be move-able I wasn't trying to break containers by
forgetting to add the const in there.

It's well documented/known that you should not put scoped_ptrs into vectors I
think. People have already been trying to use C++11 library things and figuring
out they don't work on trybots. I am not sure why this warrents a special
exception.

I think that I am happy to approve this change. Nico?

[cpu_(ooo_6.6-7.5), 2015-04-13 21:32:59]

cpu@chromium.org changed reviewers:
+ cpu@chromium.org

[cpu_(ooo_6.6-7.5), 2015-04-13 21:32:59]

VS2015 which we are trying to port to is unhappy without this change. I support
Danakj's take.

It is also the correct signature of those members ..

And there is trybots so it is not that we don't have quite many cases where you
would do something in linux that only works there.

anyhow lgtm from me.

[Nico, 2015-04-13 21:34:47]

On Fri, Apr 10, 2015 at 10:32 AM, <danakj@chromium.org> wrote:

> On 2015/04/07 17:45:43, mgraczyk wrote:
>
>> On 2015/04/07 17:00:08, Nico wrote:
>> > On 2015/04/07 05:32:59, mgraczyk wrote:
>> > > https://codereview.chromium.org/1061073004/diff/1/base/move.h
>> > > File base/move.h (right):
>> > >
>> > > https://codereview.chromium.org/1061073004/diff/1/base/
>> move.h#newcode222
>> > > base/move.h:222: type(const type&); \
>> > > On 2015/04/07 01:36:47, Nico wrote:
>> > > > On 2015/04/07 01:10:31, mgraczyk wrote:
>> > > > > This allows move-only types to be used in STL containers, as long
>> as
>>
> the
>
>> > > > > container is never copied.
>> > > >
>> > > > Only if your STL knows about move-only types, right?
>> > >
>> > > It may or may not work after this change if the library does not have
>> > move-aware
>> > > containers, but it will work in more situations with the change than
>>
> without
>
>> > > (unless I missed something thinking about this).  Specifically, it
>> will
>>
> work
>
>> > in
>> > > the case implemented by move_unittest.cc, which is new to this CL.
>> >
>> > That's worse than what we have today though, right? Since we don't allow
>>
> c++11
>
>> > library features yet, with this CL folks can do something that will only
>>
> work
>
>> on
>> > some platforms (noticeably linux, which most devs use) but which will
>> then
>> fail
>> > at trybot time.
>> >
>> > The thing in a test requires using move constructors, which aren't
>> generally
>> > allowed (http://chromium-cpp.appspot.com)
>> >
>> > I'm not sure if we should do this change at this point. danakj,
>> thougths?
>>
>
>  The client is required to write a move constructor to use the
>> MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03 macro.  All of the code
>> currently using move.h includes implementations of move constructors or
>> the
>> equivalent emulated version.  The change in this CL is in order to affect
>> similar implementations that are presupposed to have move constructors.
>>
>
>  My unit test works on all platforms, not just Linux.  Are you saying that
>>
> people
>
>> will expect STL containers to "just work" the way Container works on
>> platforms
>> without C++11 libraries or is there a different concern there?
>>
>
>
>  As for non-move-aware STLs, those will likely not have explicitly
>> defaulted
>>
> copy
>
>> constructors, so they will likely be unaffected by this change.
>>
>
> I think Nico's concern is not about breaking non-move-aware STLs, but
> rather
> making move-aware containers work when they won't on all platforms.
>
> While this is true, I'm not sure purposefully breaking them is the right
> thing
> to do. The const T& in this patch is strictly more correct AFAICT and when
> I
> converted scoped_ptr to be move-able I wasn't trying to break containers by
> forgetting to add the const in there.
>
> It's well documented/known that you should not put scoped_ptrs into
> vectors I
> think. People have already been trying to use C++11 library things and
> figuring
> out they don't work on trybots. I am not sure why this warrents a special
> exception.
>
> I think that I am happy to approve this change. Nico?
>

That's fine with me :-)


>
> https://codereview.chromium.org/1061073004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[cpu_(ooo_6.6-7.5), 2015-04-14 16:00:07]

The CQ bit was checked by cpu@chromium.org

[commit-bot: I haz the power, 2015-04-14 16:00:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1061073004/40001

[danakj, 2015-04-14 16:07:56]

LGTM

[commit-bot: I haz the power, 2015-04-14 19:46:25]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-14 19:47:22]

Patchset 3 (id:??) landed as
https://crrev.com/e6a653183ba2a1c4f24990904dfc78e32c975f5b
Cr-Commit-Position: refs/heads/master@{#325107}