Remove old C++03 move emulation code.

Remove old C++03 move emulation code.

Chrome allows the use of C++11 features now, so just use rvalue
references directly.

BUG=543901
Committed: https://crrev.com/e1b0277c20198c31b8782f567634552243182d08
Cr-Commit-Position: refs/heads/master@{#362394}

[dcheng, 2015-10-13 21:03:07]

dcheng@chromium.org changed reviewers:
+ danakj@chromium.org, jyasskin@chromium.org, rsleevi@chromium.org

[dcheng, 2015-10-13 21:03:09]

The overall CL is pretty straightforward, but I added a couple notes that I'd
love to hear some thoughts on (mostly regarding conventions).

https://codereview.chromium.org/1407443002/diff/60001/base/files/file.cc
File base/files/file.cc (left):

https://codereview.chromium.org/1407443002/diff/60001/base/files/file.cc#oldc...
base/files/file.cc:76: if (this != other.object) {
As far as I can tell, these checks were all defensive. I've changed them all to
DCHECK for now.

I've seen arguments both for and against a self-move check in the move
assignment operator. I'm open to either way, but it might be nice to have some
sort of convention about this in Chrome.

https://codereview.chromium.org/1407443002/diff/60001/base/files/file.h
File base/files/file.h (left):

https://codereview.chromium.org/1407443002/diff/60001/base/files/file.h#oldco...
base/files/file.h:171: // Move constructor for C++03 move emulation of this
type.
I opted to remove the comments of this form, since I don't think they add a lot:
I'm assuming people will understand that a constructor/assignment operator
taking a rvalue reference will use move semantics.

https://codereview.chromium.org/1407443002/diff/60001/base/move.h
File base/move.h (right):

https://codereview.chromium.org/1407443002/diff/60001/base/move.h#newcode62
base/move.h:62: #define MOVE_ONLY_TYPE_FOR_CPP_03(type) \
We should probably rename these macros, but I'd like to defer that to a separate
patch, once we're not using the old C++03 emulation anymore.

https://codereview.chromium.org/1407443002/diff/60001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/1407443002/diff/60001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:215: 
clang-format really wants this line.

https://codereview.chromium.org/1407443002/diff/60001/remoting/base/typed_buf...
File remoting/base/typed_buffer.h (left):

https://codereview.chromium.org/1407443002/diff/60001/remoting/base/typed_buf...
remoting/base/typed_buffer.h:37: TypedBuffer temp;
I'm not quite sure what the purpose of the temp here is: it should be possible
to self-move in a move constructor.

https://codereview.chromium.org/1407443002/diff/60001/remoting/base/typed_buf...
remoting/base/typed_buffer.h:51: TypedBuffer temp;
I opted to just DCHECK that this is not a self-move.

[dcheng, 2015-10-13 21:03:39]

https://codereview.chromium.org/1407443002/diff/60001/remoting/base/typed_buf...
File remoting/base/typed_buffer.h (left):

https://codereview.chromium.org/1407443002/diff/60001/remoting/base/typed_buf...
remoting/base/typed_buffer.h:37: TypedBuffer temp;
On 2015/10/13 at 21:03:09, dcheng wrote:
> I'm not quite sure what the purpose of the temp here is: it should be possible
to self-move in a move constructor.

should *not* be possible

[dcheng, 2015-10-15 21:22:28]

ping =)

[danakj, 2015-10-15 22:14:54]

oops. reviewing.

On Thu, Oct 15, 2015 at 2:22 PM, <dcheng@chromium.org> wrote:

> ping =)
>
> https://codereview.chromium.org/1407443002/
>

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

[danakj, 2015-10-15 23:35:06]

LGTM with some comments around DCHECKing mostly.

https://codereview.chromium.org/1407443002/diff/60001/base/files/file.cc
File base/files/file.cc (right):

https://codereview.chromium.org/1407443002/diff/60001/base/files/file.cc#newc...
base/files/file.cc:76: Close();
This close would be destructive if this == &other. So it's good to that the
DCHECK. Assuming it doesn't actually happen, then we should early out. :)
Without destructive behaviour like this (which I expect to be rare), DCHECK or
early out both seem overly scared and we can just ignore that and move things
from other to this, IMO.

https://codereview.chromium.org/1407443002/diff/60001/base/files/file.cc#newc...
base/files/file.cc:78: tracing_path_ = other.tracing_path_;
we should probably be move()ing all the arguments in a move assignment. TODO?

https://codereview.chromium.org/1407443002/diff/60001/base/files/file.h
File base/files/file.h (left):

https://codereview.chromium.org/1407443002/diff/60001/base/files/file.h#oldco...
base/files/file.h:171: // Move constructor for C++03 move emulation of this
type.
On 2015/10/13 21:03:09, dcheng wrote:
> I opted to remove the comments of this form, since I don't think they add a
lot:
> I'm assuming people will understand that a constructor/assignment operator
> taking a rvalue reference will use move semantics.

sgtm

https://codereview.chromium.org/1407443002/diff/60001/base/move.h
File base/move.h (right):

https://codereview.chromium.org/1407443002/diff/60001/base/move.h#newcode53
base/move.h:53: // If you include a move-only type as a field inside a class
that does not
I think this is not true with the new macro cuz the private copy constructor is
const type& not type& now. WDYT?

https://codereview.chromium.org/1407443002/diff/60001/base/move.h#newcode62
base/move.h:62: #define MOVE_ONLY_TYPE_FOR_CPP_03(type) \
On 2015/10/13 21:03:09, dcheng wrote:
> We should probably rename these macros, but I'd like to defer that to a
separate
> patch, once we're not using the old C++03 emulation anymore.

Or maybe make a base::move() and delete the macro.

https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/printing...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:123: // Move constructor for
STL.
uh maybe kill these comments? they're odd

https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_hunspell_dictionary.cc (right):

https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_hunspell_dictionary.cc:89:
DCHECK_NE(this, &other);
this looks non-destructive, can we remove the dcheck? or not i don't super mind
a DCHECK either.

https://codereview.chromium.org/1407443002/diff/60001/chrome/common/media_gal...
File chrome/common/media_galleries/picasa_types.cc (right):

https://codereview.chromium.org/1407443002/diff/60001/chrome/common/media_gal...
chrome/common/media_galleries/picasa_types.cc:79: DCHECK_NE(this, &other);
this is move()ing all the things. i don't see the point of DCHECK

https://codereview.chromium.org/1407443002/diff/60001/net/base/file_stream_co...
File net/base/file_stream_context.cc (right):

https://codereview.chromium.org/1407443002/diff/60001/net/base/file_stream_co...
net/base/file_stream_context.cc:62: DCHECK_NE(this, &other);
also not destructive, remove?

https://codereview.chromium.org/1407443002/diff/60001/remoting/base/typed_buf...
File remoting/base/typed_buffer.h (right):

https://codereview.chromium.org/1407443002/diff/60001/remoting/base/typed_buf...
remoting/base/typed_buffer.h:45: DCHECK_NE(this, &rvalue);
Swap would handle *this ok? No dcheck?

[danakj, 2015-10-15 23:37:06]

Shall we make a bug for getting rid of the move emulation macros etc entirely
and attach this to it? There's at least 3 patches I see doing this stuff.

[dcheng, 2015-10-16 00:40:01]

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

[dcheng, 2015-10-16 00:40:01]

+thakis because C++11 library support managed to creep into this review
(pdf_to_emf_converter.cc)

https://codereview.chromium.org/1407443002/diff/60001/base/files/file.cc
File base/files/file.cc (right):

https://codereview.chromium.org/1407443002/diff/60001/base/files/file.cc#newc...
base/files/file.cc:78: tracing_path_ = other.tracing_path_;
On 2015/10/15 at 23:35:05, danakj wrote:
> we should probably be move()ing all the arguments in a move assignment. TODO?

It seems weird to std::move() an integral type.

FilePath isn't moveable today and it might be in the future, but this seems like
it'd be a lot of TODOs to add. Maybe this should just be something that a
diagnostic could detect? "Field with moveable type copied instead of moved in
move constructor".

https://codereview.chromium.org/1407443002/diff/60001/base/move.h
File base/move.h (right):

https://codereview.chromium.org/1407443002/diff/60001/base/move.h#newcode53
base/move.h:53: // If you include a move-only type as a field inside a class
that does not
On 2015/10/15 at 23:35:06, danakj wrote:
> I think this is not true with the new macro cuz the private copy constructor
is const type& not type& now. WDYT?

Yeah, I think we shouldn't need this anymore. Removed.

I also mark them as deleted, because why not.

https://codereview.chromium.org/1407443002/diff/60001/base/move.h#newcode62
base/move.h:62: #define MOVE_ONLY_TYPE_FOR_CPP_03(type) \
On 2015/10/15 at 23:35:06, danakj wrote:
> On 2015/10/13 21:03:09, dcheng wrote:
> > We should probably rename these macros, but I'd like to defer that to a
separate
> > patch, once we're not using the old C++03 emulation anymore.
> 
> Or maybe make a base::move() and delete the macro.

That's an option too, but I think that would involve a bunch of #include
juggling that I don't really want to deal with, especially since we'll have to
change the include again when we switch to std::move.

https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/printing...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:123: // Move constructor for
STL.
On 2015/10/15 at 23:35:06, danakj wrote:
> uh maybe kill these comments? they're odd

Ah, I meant to comment on this.

So it's really weird that this comment would say "move constructor for STL",
because there's no way STL containers would explicitly support our custom RValue
emulation. I think this happens to work because std::queue never reallocates
internal buffers the way std::vector would, so it never needed to actually move
elements internally. And std::queue::push(GetPageCallbackData(...)) just
happened to tickle the RValue conversion path, allowing a move to take place.

Of course, the problem is we're now using C++11 library features and we're not
supposed to... so I'm not quite sure how to resolve this. +thakis

https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_hunspell_dictionary.cc (right):

https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_hunspell_dictionary.cc:89:
DCHECK_NE(this, &other);
On 2015/10/15 at 23:35:06, danakj wrote:
> this looks non-destructive, can we remove the dcheck? or not i don't super
mind a DCHECK either.

  file = other.file.Pass()

is destructive.

That being said, File::operator=(File&&) will DCHECK on a self-move here. So the
question is if it's OK for a class embedding a type to depend on the fact that a
self-move of the other type should/will DCHECK. My sense is probably? But I want
to confirm before actually making this change.

https://codereview.chromium.org/1407443002/diff/60001/chrome/common/media_gal...
File chrome/common/media_galleries/picasa_types.cc (right):

https://codereview.chromium.org/1407443002/diff/60001/chrome/common/media_gal...
chrome/common/media_galleries/picasa_types.cc:79: DCHECK_NE(this, &other);
On 2015/10/15 at 23:35:06, danakj wrote:
> this is move()ing all the things. i don't see the point of DCHECK

See above.

https://codereview.chromium.org/1407443002/diff/60001/net/base/file_stream_co...
File net/base/file_stream_context.cc (right):

https://codereview.chromium.org/1407443002/diff/60001/net/base/file_stream_co...
net/base/file_stream_context.cc:62: DCHECK_NE(this, &other);
On 2015/10/15 at 23:35:06, danakj wrote:
> also not destructive, remove?

See above.

https://codereview.chromium.org/1407443002/diff/60001/remoting/base/typed_buf...
File remoting/base/typed_buffer.h (right):

https://codereview.chromium.org/1407443002/diff/60001/remoting/base/typed_buf...
remoting/base/typed_buffer.h:45: DCHECK_NE(this, &rvalue);
On 2015/10/15 at 23:35:06, danakj wrote:
> Swap would handle *this ok? No dcheck?

Done.

[danakj, 2015-10-16 00:44:24]

https://codereview.chromium.org/1407443002/diff/60001/base/files/file.cc
File base/files/file.cc (right):

https://codereview.chromium.org/1407443002/diff/60001/base/files/file.cc#newc...
base/files/file.cc:78: tracing_path_ = other.tracing_path_;
On 2015/10/16 00:40:01, dcheng wrote:
> On 2015/10/15 at 23:35:05, danakj wrote:
> > we should probably be move()ing all the arguments in a move assignment.
TODO?
> 
> It seems weird to std::move() an integral type.
> 
> FilePath isn't moveable today and it might be in the future, but this seems
like
> it'd be a lot of TODOs to add. Maybe this should just be something that a
> diagnostic could detect? "Field with moveable type copied instead of moved in
> move constructor".

It's okay if it's not moveable, the code would work fine if it did. move() casts
to an rvalue but isn't guaranteed to not copy. For ints ya prob doesnt make
sense.

https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/printing...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:123: // Move constructor for
STL.
On 2015/10/16 00:40:01, dcheng wrote:
> On 2015/10/15 at 23:35:06, danakj wrote:
> > uh maybe kill these comments? they're odd
> 
> Ah, I meant to comment on this.
> 
> So it's really weird that this comment would say "move constructor for STL",
> because there's no way STL containers would explicitly support our custom
RValue
> emulation. I think this happens to work because std::queue never reallocates
> internal buffers the way std::vector would, so it never needed to actually
move
> elements internally. And std::queue::push(GetPageCallbackData(...)) just
> happened to tickle the RValue conversion path, allowing a move to take place.
> 
> Of course, the problem is we're now using C++11 library features and we're not
> supposed to... so I'm not quite sure how to resolve this. +thakis

Uh, well, the old RValue things would not have been used by STL, cuz you have to
Pass() to actually move. If RVO happens to kick in more that seems okay?

https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_hunspell_dictionary.cc (right):

https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_hunspell_dictionary.cc:89:
DCHECK_NE(this, &other);
On 2015/10/16 00:40:01, dcheng wrote:
> On 2015/10/15 at 23:35:06, danakj wrote:
> > this looks non-destructive, can we remove the dcheck? or not i don't super
> mind a DCHECK either.
> 
>   file = other.file.Pass()
> 
> is destructive.
> 
> That being said, File::operator=(File&&) will DCHECK on a self-move here. So
the
> question is if it's OK for a class embedding a type to depend on the fact that
a
> self-move of the other type should/will DCHECK. My sense is probably? But I
want
> to confirm before actually making this change.

I think that's the right way to do it ya.

[dcheng, 2015-10-16 01:30:30]

My brain is tired so I'll address the remaining comments tomorrow.

https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/printing...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:123: // Move constructor for
STL.
On 2015/10/16 at 00:44:24, danakj wrote:
> On 2015/10/16 00:40:01, dcheng wrote:
> > On 2015/10/15 at 23:35:06, danakj wrote:
> > > uh maybe kill these comments? they're odd
> > 
> > Ah, I meant to comment on this.
> > 
> > So it's really weird that this comment would say "move constructor for STL",
> > because there's no way STL containers would explicitly support our custom
RValue
> > emulation. I think this happens to work because std::queue never reallocates
> > internal buffers the way std::vector would, so it never needed to actually
move
> > elements internally. And std::queue::push(GetPageCallbackData(...)) just
> > happened to tickle the RValue conversion path, allowing a move to take
place.
> > 
> > Of course, the problem is we're now using C++11 library features and we're
not
> > supposed to... so I'm not quite sure how to resolve this. +thakis
> 
> Uh, well, the old RValue things would not have been used by STL, cuz you have
to Pass() to actually move. If RVO happens to kick in more that seems okay?

Thinking about this more, my original line of reasoning is probably completely
wrong.

... that being said, I think that this block of code must already depend on
C++11 library features.

In C++03, push has the signature std::queue<T>::push(const T&). One condition
for copy elision is:
  "When a nameless temporary, not bound to any references, would be moved or
copied into an object of the same type (ignoring top-level cv-qualification),
the copy/move is omitted" [1]

But passing a T to a const T& would not be a copy or a move of a nameless
temporary. Following the chain of stuff, C++03 eventually uses placement new to
copy construct the (no longer nameless) argument:

      // From libstdc++:
      void 
      construct(pointer __p, const _Tp& __val) 
      { ::new((void *)__p) _Tp(__val); }

Copy elision would also not apply here. Since __val is a const lvalue reference:

  1. it can't bind to the copy constructor (which takes a mutable reference)
  2. we can't use any user-defined conversions, because they are all non-const.

Thus, this shouldn't even compile in C++03.

However, the C++11 version uses perfect forwarding:
      template<typename _Up, typename... _Args>
        void
        construct(_Up* __p, _Args&&... __args)
	{ ::new((void *)__p) _Up(std::forward<_Args>(__args)...); }

and in C++11, there's an additional overload: std::queue<T>::push(T&&). Since we
have a temporary, it binds to the T&& overload. Then, inside construct(...),
Args&& expands to GetPageCallbackData&& &&, which collapses to
GetPageCallbackData&&. At this point, I think the rule about using one
user-defined conversion kicks in, operator RValue() gets invoked (since |args|
is not const), and GetPageCallbackData is constructed in place using the RValue
constructor.

[1] http://en.cppreference.com/w/cpp/language/copy_elision

[Nico, 2015-10-16 04:06:46]

On Thu, Oct 15, 2015 at 6:30 PM, <dcheng@chromium.org> wrote:

> My brain is tired so I'll address the remaining comments tomorrow.
>
>
>
>
https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/printing...
> File chrome/browser/printing/pdf_to_emf_converter.cc (right):
>
>
>
https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/printing...
> chrome/browser/printing/pdf_to_emf_converter.cc:123: // Move constructor
> for STL.
> On 2015/10/16 at 00:44:24, danakj wrote:
>
>> On 2015/10/16 00:40:01, dcheng wrote:
>> > On 2015/10/15 at 23:35:06, danakj wrote:
>> > > uh maybe kill these comments? they're odd
>> >
>> > Ah, I meant to comment on this.
>> >
>> > So it's really weird that this comment would say "move constructor
>>
> for STL",
>
>> > because there's no way STL containers would explicitly support our
>>
> custom RValue
>
>> > emulation. I think this happens to work because std::queue never
>>
> reallocates
>
>> > internal buffers the way std::vector would, so it never needed to
>>
> actually move
>
>> > elements internally. And std::queue::push(GetPageCallbackData(...))
>>
> just
>
>> > happened to tickle the RValue conversion path, allowing a move to
>>
> take place.
>
>> >
>> > Of course, the problem is we're now using C++11 library features and
>>
> we're not
>
>> > supposed to... so I'm not quite sure how to resolve this. +thakis
>>
>
> Uh, well, the old RValue things would not have been used by STL, cuz
>>
> you have to Pass() to actually move. If RVO happens to kick in more that
> seems okay?
>
> Thinking about this more, my original line of reasoning is probably
> completely wrong.
>
> ... that being said, I think that this block of code must already depend
> on C++11 library features.
>
> In C++03, push has the signature std::queue<T>::push(const T&). One
> condition for copy elision is:
>   "When a nameless temporary, not bound to any references, would be
> moved or copied into an object of the same type (ignoring top-level
> cv-qualification), the copy/move is omitted" [1]
>
> But passing a T to a const T& would not be a copy or a move of a
> nameless temporary. Following the chain of stuff, C++03 eventually uses
> placement new to copy construct the (no longer nameless) argument:
>
>       // From libstdc++:
>       void
>       construct(pointer __p, const _Tp& __val)
>       { ::new((void *)__p) _Tp(__val); }
>
> Copy elision would also not apply here. Since __val is a const lvalue
> reference:
>
>   1. it can't bind to the copy constructor (which takes a mutable
> reference)
>   2. we can't use any user-defined conversions, because they are all
> non-const.
>
> Thus, this shouldn't even compile in C++03.
>
> However, the C++11 version uses perfect forwarding:
>       template<typename _Up, typename... _Args>
>         void
>         construct(_Up* __p, _Args&&... __args)
>         { ::new((void *)__p) _Up(std::forward<_Args>(__args)...); }
>

I doubt this version exists in libstdc++ 4.2 (?)


>
> and in C++11, there's an additional overload: std::queue<T>::push(T&&).
> Since we have a temporary, it binds to the T&& overload. Then, inside
> construct(...), Args&& expands to GetPageCallbackData&& &&, which
> collapses to GetPageCallbackData&&. At this point, I think the rule
> about using one user-defined conversion kicks in, operator RValue() gets
> invoked (since |args| is not const), and GetPageCallbackData is
> constructed in place using the RValue constructor.
>
> [1] http://en.cppreference.com/w/cpp/language/copy_elision
>
> https://codereview.chromium.org/1407443002/
>

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

[Nico, 2015-10-16 04:29:18]

Can we wait with doing this until we have library support?

Also, keeping a macro around makes the move stuff look more special, which means
people might pay more attention to the http://chromium-cpp.appspot.com/ bit that
says that move types aren't generally allowed.

(Also, according to
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/xMscQuYBwyc...
you get to explain the difference between std::move and std::forward, and to
make a case for most engs on chrome team being able to explain this :-) )

[dcheng, 2015-10-16 05:03:51]

On 2015/10/16 at 04:29:18, thakis wrote:
> Can we wait with doing this until we have library support?

The thing is, lack of library support isn't really causing people to avoid this
either. RValue emulation already "works" in STL containers if you have to have
the T&& overloads (https://codereview.chromium.org/1410703002/#ps1). There are
already people using this, as I discovered in this patch. If we use an actual
move constructor (https://codereview.chromium.org/1410703002/#ps20001), nothing
changes: platforms which have a T&& overload still work, and platforms which
don't still crash and burn.

> 
> Also, keeping a macro around makes the move stuff look more special, which
means people might pay more attention to the http://chromium-cpp.appspot.com/
bit that says that move types aren't generally allowed.

If move semantics aren't generally allowed, shouldn't we be discouraging people
from adding them, whether it's based on RValue or rvalue references? And if they
are allowed, then why not let developers start getting exposure to rvalue
references in limited scenarios like this? It's more or less what the current
Google C++ style guide permits for rvalue reference usage anyway.

Also, I'm not planning on removing the macro: I'd just like to consolidate the
two move-only macros for now.

> 
> (Also, according to
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/xMscQuYBwyc...
you get to explain the difference between std::move and std::forward, and to
make a case for most engs on chrome team being able to explain this :-) )

[danakj, 2015-10-16 18:36:28]

On Thu, Oct 15, 2015 at 10:03 PM, <dcheng@chromium.org> wrote:

> On 2015/10/16 at 04:29:18, thakis wrote:
>
>> Can we wait with doing this until we have library support?
>>
>
> The thing is, lack of library support isn't really causing people to avoid
> this
> either. RValue emulation already "works" in STL containers if you have to
> have
> the T&& overloads (https://codereview.chromium.org/1410703002/#ps1).
> There are
> already people using this, as I discovered in this patch. If we use an
> actual
> move constructor (https://codereview.chromium.org/1410703002/#ps20001),
> nothing
> changes: platforms which have a T&& overload still work, and platforms
> which
> don't still crash and burn.
>
>
> Also, keeping a macro around makes the move stuff look more special, which
>>
> means people might pay more attention to the
> http://chromium-cpp.appspot.com/
> bit that says that move types aren't generally allowed.
>
> If move semantics aren't generally allowed, shouldn't we be discouraging
> people
> from adding them, whether it's based on RValue or rvalue references? And
> if they
> are allowed, then why not let developers start getting exposure to rvalue
> references in limited scenarios like this? It's more or less what the
> current
> Google C++ style guide permits for rvalue reference usage anyway.
>

Yeh.. this. It was never the case that we wanted to prevent people from
making move-only types was it? We didn't warn people to not use the macro.
I don't see an advantage to leaving the macro implementation with the 03
emulation now.

The guide says we don't want people writing code with rvalue references,
but they can use the base macro and make a moveable type with Pass(). My
reading of the guide is basically that we don't want them to write move()
or forward() yet, which this CL does not do. Writing move
constructor/assignment only lets you use RVO more effectively, or lets you
use our macro's Pass() for a move-only type which doesn't seem like a more
dangerous world than the move-emulation one to me.

(If we're going to use unique_ptr, devs are going to have to use move(), so
IMO we can't get out of this state soon enough. Vlad has started helping
out with transcribing the effective c++ talk about move/forward, so we can
make a best effort toward helping chrome devs understand these things. At
some point we we'll have to let people walk on their own.)


>
> Also, I'm not planning on removing the macro: I'd just like to consolidate
> the
> two move-only macros for now.
>
>
>
> (Also, according to
>>
>
>
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/xMscQuYBwyc...
> you get to explain the difference between std::move and std::forward, and
> to
> make a case for most engs on chrome team being able to explain this :-) )
>
> https://codereview.chromium.org/1407443002/
>

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

[dcheng, 2015-10-19 21:29:50]

https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/printing...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:123: // Move constructor for
STL.
On 2015/10/15 at 23:35:06, danakj wrote:
> uh maybe kill these comments? they're odd

Done.

https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_hunspell_dictionary.cc (right):

https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_hunspell_dictionary.cc:89:
DCHECK_NE(this, &other);
On 2015/10/16 at 00:44:24, danakj wrote:
> On 2015/10/16 00:40:01, dcheng wrote:
> > On 2015/10/15 at 23:35:06, danakj wrote:
> > > this looks non-destructive, can we remove the dcheck? or not i don't super
> > mind a DCHECK either.
> > 
> >   file = other.file.Pass()
> > 
> > is destructive.
> > 
> > That being said, File::operator=(File&&) will DCHECK on a self-move here. So
the
> > question is if it's OK for a class embedding a type to depend on the fact
that a
> > self-move of the other type should/will DCHECK. My sense is probably? But I
want
> > to confirm before actually making this change.
> 
> I think that's the right way to do it ya.

Done.

https://codereview.chromium.org/1407443002/diff/60001/chrome/common/media_gal...
File chrome/common/media_galleries/picasa_types.cc (right):

https://codereview.chromium.org/1407443002/diff/60001/chrome/common/media_gal...
chrome/common/media_galleries/picasa_types.cc:79: DCHECK_NE(this, &other);
On 2015/10/16 at 00:40:01, dcheng wrote:
> On 2015/10/15 at 23:35:06, danakj wrote:
> > this is move()ing all the things. i don't see the point of DCHECK
> 
> See above.

Done.

https://codereview.chromium.org/1407443002/diff/60001/net/base/file_stream_co...
File net/base/file_stream_context.cc (right):

https://codereview.chromium.org/1407443002/diff/60001/net/base/file_stream_co...
net/base/file_stream_context.cc:62: DCHECK_NE(this, &other);
On 2015/10/16 at 00:40:01, dcheng wrote:
> On 2015/10/15 at 23:35:06, danakj wrote:
> > also not destructive, remove?
> 
> See above.

Done.

[danakj, 2015-10-19 21:49:24]

LGTM % thakis

https://codereview.chromium.org/1407443002/diff/60001/base/move.h
File base/move.h (right):

https://codereview.chromium.org/1407443002/diff/60001/base/move.h#newcode53
base/move.h:53: // If you include a move-only type as a field inside a class
that does not
On 2015/10/16 00:40:01, dcheng wrote:
> On 2015/10/15 at 23:35:06, danakj wrote:
> > I think this is not true with the new macro cuz the private copy constructor
> is const type& not type& now. WDYT?
> 
> Yeah, I think we shouldn't need this anymore. Removed.
> 
> I also mark them as deleted, because why not.

+1

https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/printing...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/1407443002/diff/60001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:123: // Move constructor for
STL.
On 2015/10/16 01:30:29, dcheng wrote:
> On 2015/10/16 at 00:44:24, danakj wrote:
> > On 2015/10/16 00:40:01, dcheng wrote:
> > > On 2015/10/15 at 23:35:06, danakj wrote:
> > > > uh maybe kill these comments? they're odd
> > > 
> > > Ah, I meant to comment on this.
> > > 
> > > So it's really weird that this comment would say "move constructor for
STL",
> > > because there's no way STL containers would explicitly support our custom
> RValue
> > > emulation. I think this happens to work because std::queue never
reallocates
> > > internal buffers the way std::vector would, so it never needed to actually
> move
> > > elements internally. And std::queue::push(GetPageCallbackData(...)) just
> > > happened to tickle the RValue conversion path, allowing a move to take
> place.
> > > 
> > > Of course, the problem is we're now using C++11 library features and we're
> not
> > > supposed to... so I'm not quite sure how to resolve this. +thakis
> > 
> > Uh, well, the old RValue things would not have been used by STL, cuz you
have
> to Pass() to actually move. If RVO happens to kick in more that seems okay?
> 
> Thinking about this more, my original line of reasoning is probably completely
> wrong.
> 
> ... that being said, I think that this block of code must already depend on
> C++11 library features.
> 
> In C++03, push has the signature std::queue<T>::push(const T&). One condition
> for copy elision is:
>   "When a nameless temporary, not bound to any references, would be moved or
> copied into an object of the same type (ignoring top-level cv-qualification),
> the copy/move is omitted" [1]
> 
> But passing a T to a const T& would not be a copy or a move of a nameless
> temporary. Following the chain of stuff, C++03 eventually uses placement new
to
> copy construct the (no longer nameless) argument:
> 
>       // From libstdc++:
>       void 
>       construct(pointer __p, const _Tp& __val) 
>       { ::new((void *)__p) _Tp(__val); }
> 
> Copy elision would also not apply here. Since __val is a const lvalue
reference:
> 
>   1. it can't bind to the copy constructor (which takes a mutable reference)
>   2. we can't use any user-defined conversions, because they are all
non-const.
> 
> Thus, this shouldn't even compile in C++03.
> 
> However, the C++11 version uses perfect forwarding:
>       template<typename _Up, typename... _Args>
>         void
>         construct(_Up* __p, _Args&&... __args)
> 	{ ::new((void *)__p) _Up(std::forward<_Args>(__args)...); }
> 
> and in C++11, there's an additional overload: std::queue<T>::push(T&&). Since
we
> have a temporary, it binds to the T&& overload. Then, inside construct(...),
> Args&& expands to GetPageCallbackData&& &&, which collapses to
> GetPageCallbackData&&. At this point, I think the rule about using one
> user-defined conversion kicks in, operator RValue() gets invoked (since |args|
> is not const), and GetPageCallbackData is constructed in place using the
RValue
> constructor.
> 
> [1] http://en.cppreference.com/w/cpp/language/copy_elision

Wow ya, I agree. The problem is they are putting a move-only type into an STL
container. And whether they do that with our emulation via T::Rvalue or not,
it's possible to do as long as the container doesn't try to move it again later?

We should remove the move-only bits here, but this class is holding a (callback
containing a) scoped_ptr. So a more significant refactor would actually be
needed.

This code is only built on windows which is why it is working:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/BUI...

Maybe just adding a scary TODO comment about at the call to push that it is
pushing a move-only type which is not possible on all platforms and it is not
allowed.. and we can remove it one day?

[Nico, 2015-10-19 21:50:56]

let's please wait with this, as requested above

[dcheng, 2015-10-19 23:02:00]

On 2015/10/19 at 21:50:56, thakis wrote:
> let's please wait with this, as requested above

Say someone wanted to make C++11 on Mac happen. Is there any opportunity for
20%ers to do so, or will that not help make things happen faster?

[Nico, 2015-10-19 23:08:56]

On 2015/10/19 23:02:00, dcheng wrote:
> On 2015/10/19 at 21:50:56, thakis wrote:
> > let's please wait with this, as requested above
> 
> Say someone wanted to make C++11 on Mac happen. Is there any opportunity for
> 20%ers to do so, or will that not help make things happen faster?

It's pretty close to done now. (Nobody wanted to help for ~1 year, and then in
the last 2 weeks three people popped up.)

[dcheng, 2015-11-11 01:53:47]

Given https://codereview.chromium.org/1433013004, is this something we can
reconsider landing now?

[Nico, 2015-11-11 01:59:47]

Please wait until library features are ready, as said above. As you can
see, that's imminent.
On Nov 10, 2015 5:53 PM, <dcheng@chromium.org> wrote:

> Given https://codereview.chromium.org/1433013004, is this something we can
> reconsider landing now?
>
> https://codereview.chromium.org/1407443002/
>

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

[Vitaly Buka (NO REVIEWS), 2015-11-17 01:37:22]

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

[Vitaly Buka (NO REVIEWS), 2015-11-17 01:37:22]

LGTM chrome/browser/printing/pdf_to_emf_converter.cc

[dcheng, 2015-11-19 00:24:57]

Now that std::move() is allowed, can I go ahead? =)

[danakj, 2015-11-19 00:29:41]

On Wed, Nov 18, 2015 at 4:24 PM, <dcheng@chromium.org> wrote:

> Now that std::move() is allowed, can I go ahead? =)
>

Move constructors still aren't allowed without styleguide owner approval...
But I do think we should eliminate the old macros. I don't think they are
themselves adding value. And I don't think this will cause revolt against
the move constructor rule until we sort that out.

WDYT Nico?

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2015-11-19 00:31:40]

On 2015/11/19 at 00:29:41, danakj wrote:
> On Wed, Nov 18, 2015 at 4:24 PM, <dcheng@chromium.org> wrote:
> 
> > Now that std::move() is allowed, can I go ahead? =)
> >
> 
> Move constructors still aren't allowed without styleguide owner approval...
> But I do think we should eliminate the old macros. I don't think they are
> themselves adding value. And I don't think this will cause revolt against
> the move constructor rule until we sort that out.
> 
> WDYT Nico?
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
> 

Hmm, I'm OK with waiting. For some reason I thought this was blocked on
std::move(), but it seems like the rule of 'rvalue references for move
ctor/assignment' is still in discussion.

[Nico, 2015-11-26 03:06:10]

alright, let's do this.

Once this is in, I suppose we can soon allow move ctors/op= in general (with the
comment repeating
http://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types 's
advice that most types should be either copyable or movable but usually not
both, and recommending this macro for deleting the copy ctor for movable types).

https://codereview.chromium.org/1407443002/diff/180001/base/move.h
File base/move.h (right):

https://codereview.chromium.org/1407443002/diff/180001/base/move.h#newcode33
base/move.h:33: //    MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03(type);
We should get rid of the _FOR_CPP_03 bit now, right?

https://codereview.chromium.org/1407443002/diff/180001/content/child/scoped_w...
File content/child/scoped_web_callbacks.h (right):

https://codereview.chromium.org/1407443002/diff/180001/content/child/scoped_w...
content/child/scoped_web_callbacks.h:83: ScopedWebCallbacks(ScopedWebCallbacks&&
other) { *this = other.Pass(); }
consider std::move() instead of Pass()

[dcheng, 2015-11-26 03:10:41]

https://codereview.chromium.org/1407443002/diff/180001/base/move.h
File base/move.h (right):

https://codereview.chromium.org/1407443002/diff/180001/base/move.h#newcode33
base/move.h:33: //    MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03(type);
On 2015/11/26 at 03:06:10, Nico wrote:
> We should get rid of the _FOR_CPP_03 bit now, right?

Yeah, I wanted to do the actual macro rename in a separate patch, just to keep
things focused. Let me know if that's OK.

https://codereview.chromium.org/1407443002/diff/180001/content/child/scoped_w...
File content/child/scoped_web_callbacks.h (right):

https://codereview.chromium.org/1407443002/diff/180001/content/child/scoped_w...
content/child/scoped_web_callbacks.h:83: ScopedWebCallbacks(ScopedWebCallbacks&&
other) { *this = other.Pass(); }
On 2015/11/26 at 03:06:10, Nico wrote:
> consider std::move() instead of Pass()

Done here and elsewhere (I'll upload this as soon as my current build for
another branch finishes).

[Nico, 2015-11-26 03:21:44]

https://codereview.chromium.org/1407443002/diff/180001/base/move.h
File base/move.h (right):

https://codereview.chromium.org/1407443002/diff/180001/base/move.h#newcode33
base/move.h:33: //    MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03(type);
On 2015/11/26 03:10:41, dcheng wrote:
> On 2015/11/26 at 03:06:10, Nico wrote:
> > We should get rid of the _FOR_CPP_03 bit now, right?
> 
> Yeah, I wanted to do the actual macro rename in a separate patch, just to keep
> things focused. Let me know if that's OK.

I think it kind of makes sense in this patch as it removes the 03 functionality
of the macro and you're touching all files using the macro already to update the
ctors. It seems like the same semantic action to me.

But if you feel strongly about doing two patches, I suppose I can live with that
:-)

[dcheng, 2015-11-26 03:41:13]

On 2015/11/26 at 03:21:44, thakis wrote:
> https://codereview.chromium.org/1407443002/diff/180001/base/move.h
> File base/move.h (right):
> 
> https://codereview.chromium.org/1407443002/diff/180001/base/move.h#newcode33
> base/move.h:33: //    MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03(type);
> On 2015/11/26 03:10:41, dcheng wrote:
> > On 2015/11/26 at 03:06:10, Nico wrote:
> > > We should get rid of the _FOR_CPP_03 bit now, right?
> > 
> > Yeah, I wanted to do the actual macro rename in a separate patch, just to
keep
> > things focused. Let me know if that's OK.
> 
> I think it kind of makes sense in this patch as it removes the 03
functionality of the macro and you're touching all files using the macro already
to update the ctors. It seems like the same semantic action to me.
> 
> But if you feel strongly about doing two patches, I suppose I can live with
that :-)

I guess I just don't want to touch the other places that already use
MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03 in this patch.

I could introduce a new macro without the _FOR_CPP_03 suffix and use that in
this patch, if that works for you. Then another patch to fix up the old ones?

[dcheng, 2015-11-30 18:18:23]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-11-30 18:18:24]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
vitalybuka@chromium.org
Link to the patchset: https://codereview.chromium.org/1407443002/#ps220001
(title: "std::move and reflow")

[dcheng, 2015-11-30 18:18:28]

The CQ bit was unchecked by dcheng@chromium.org

[dcheng, 2015-11-30 18:18:45]

dcheng@chromium.org changed reviewers:
+ brettw@chromium.org

[dcheng, 2015-11-30 18:18:46]

+brettw for global OWNERS approval

[danakj, 2015-11-30 18:33:51]

On Wed, Nov 25, 2015 at 7:41 PM, <dcheng@chromium.org> wrote:

> On 2015/11/26 at 03:21:44, thakis wrote:
>
>> https://codereview.chromium.org/1407443002/diff/180001/base/move.h
>> File base/move.h (right):
>>
>
>
>> https://codereview.chromium.org/1407443002/diff/180001/base/move.h#newcode33
>> base/move.h:33: //
>> MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03(type);
>> On 2015/11/26 03:10:41, dcheng wrote:
>> > On 2015/11/26 at 03:06:10, Nico wrote:
>> > > We should get rid of the _FOR_CPP_03 bit now, right?
>> >
>> > Yeah, I wanted to do the actual macro rename in a separate patch, just
>> to
>>
> keep
>
>> > things focused. Let me know if that's OK.
>>
>
> I think it kind of makes sense in this patch as it removes the 03
>>
> functionality of the macro and you're touching all files using the macro
> already
> to update the ctors. It seems like the same semantic action to me.
>
> But if you feel strongly about doing two patches, I suppose I can live with
>>
> that :-)
>
> I guess I just don't want to touch the other places that already use
> MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03 in this patch.
>
> I could introduce a new macro without the _FOR_CPP_03 suffix and use that
> in
> this patch, if that works for you. Then another patch to fix up the old
> ones?
>

I was thinking about renaming this macro and was going to call it
DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND or something, and put it in
macros.h beside DISALLOW_COPY_AND_ASSIGN, as it will be replaced with the
latter once Bind works with proper forwarding.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2015-11-30 18:34:50]

On 2015/11/30 at 18:33:51, danakj wrote:
> On Wed, Nov 25, 2015 at 7:41 PM, <dcheng@chromium.org> wrote:
> 
> > On 2015/11/26 at 03:21:44, thakis wrote:
> >
> >> https://codereview.chromium.org/1407443002/diff/180001/base/move.h
> >> File base/move.h (right):
> >>
> >
> >
> >>
https://codereview.chromium.org/1407443002/diff/180001/base/move.h#newcode33
> >> base/move.h:33: //
> >> MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03(type);
> >> On 2015/11/26 03:10:41, dcheng wrote:
> >> > On 2015/11/26 at 03:06:10, Nico wrote:
> >> > > We should get rid of the _FOR_CPP_03 bit now, right?
> >> >
> >> > Yeah, I wanted to do the actual macro rename in a separate patch, just
> >> to
> >>
> > keep
> >
> >> > things focused. Let me know if that's OK.
> >>
> >
> > I think it kind of makes sense in this patch as it removes the 03
> >>
> > functionality of the macro and you're touching all files using the macro
> > already
> > to update the ctors. It seems like the same semantic action to me.
> >
> > But if you feel strongly about doing two patches, I suppose I can live with
> >>
> > that :-)
> >
> > I guess I just don't want to touch the other places that already use
> > MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03 in this patch.
> >
> > I could introduce a new macro without the _FOR_CPP_03 suffix and use that
> > in
> > this patch, if that works for you. Then another patch to fix up the old
> > ones?
> >
> 
> I was thinking about renaming this macro and was going to call it
> DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND or something, and put it in
> macros.h beside DISALLOW_COPY_AND_ASSIGN, as it will be replaced with the
> latter once Bind works with proper forwarding.

Sure, that seems fine. Any chance that can be ordered to land after this patch?
;-)

> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
>

[danakj, 2015-11-30 18:45:55]

On Mon, Nov 30, 2015 at 10:34 AM, <dcheng@chromium.org> wrote:

> On 2015/11/30 at 18:33:51, danakj wrote:
>
>> On Wed, Nov 25, 2015 at 7:41 PM, <dcheng@chromium.org> wrote:
>>
>
> > On 2015/11/26 at 03:21:44, thakis wrote:
>> >
>> >> https://codereview.chromium.org/1407443002/diff/180001/base/move.h
>> >> File base/move.h (right):
>> >>
>> >
>> >
>> >>
>>
>
> https://codereview.chromium.org/1407443002/diff/180001/base/move.h#newcode33
>
>> >> base/move.h:33: //
>> >> MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03(type);
>> >> On 2015/11/26 03:10:41, dcheng wrote:
>> >> > On 2015/11/26 at 03:06:10, Nico wrote:
>> >> > > We should get rid of the _FOR_CPP_03 bit now, right?
>> >> >
>> >> > Yeah, I wanted to do the actual macro rename in a separate patch,
>> just
>> >> to
>> >>
>> > keep
>> >
>> >> > things focused. Let me know if that's OK.
>> >>
>> >
>> > I think it kind of makes sense in this patch as it removes the 03
>> >>
>> > functionality of the macro and you're touching all files using the macro
>> > already
>> > to update the ctors. It seems like the same semantic action to me.
>> >
>> > But if you feel strongly about doing two patches, I suppose I can live
>> with
>> >>
>> > that :-)
>> >
>> > I guess I just don't want to touch the other places that already use
>> > MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03 in this patch.
>> >
>> > I could introduce a new macro without the _FOR_CPP_03 suffix and use
>> that
>> > in
>> > this patch, if that works for you. Then another patch to fix up the old
>> > ones?
>> >
>>
>
> I was thinking about renaming this macro and was going to call it
>> DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND or something, and put it in
>> macros.h beside DISALLOW_COPY_AND_ASSIGN, as it will be replaced with the
>> latter once Bind works with proper forwarding.
>>
>
> Sure, that seems fine. Any chance that can be ordered to land after this
> patch?
> ;-)
>

Oh most definitely. I've been trying to convert existing uses to
DISALLOW_COPY_AND_ASSIGN where they aren't used with Bind first, also.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brettw, 2015-12-01 06:34:16]

rs lgtm

[dcheng, 2015-12-01 08:44:07]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2015-12-01 08:44:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1407443002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1407443002/220001

[commit-bot: I haz the power, 2015-12-01 12:09:59]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-12-01 12:10:50]

Description was changed from

==========
Remove old C++03 move emulation code.

Chrome allows the use of C++11 features now, so just use rvalue
references directly.

BUG=543901
==========

to

==========
Remove old C++03 move emulation code.

Chrome allows the use of C++11 features now, so just use rvalue
references directly.

BUG=543901

Committed: https://crrev.com/e1b0277c20198c31b8782f567634552243182d08
Cr-Commit-Position: refs/heads/master@{#362394}
==========

[commit-bot: I haz the power, 2015-12-01 12:10:51]

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