Add move constructor and assignment operator to RefPtr class

Add move constructor and assignment operator to RefPtr class

Add move constructor and assignment operator to RefPtr class. As a result, we
can now use the implicitly defined move constructor and assignment operator for
Atomic String. Also, other classes with RefPtr data members like CString now
get a better default move constructor and assignment operator.

The release binary is ~21Kb smaller, there is no diff on the generated asm
for CString and AtomicString. However, there is a small diff for WTFString.o:
http://pastebin.com/RTzX23JV

The generated assembly for "CString WTF::String::latin1() const" and
"CString WTF::String::ascii() const" is slightly smaller, likely due to
CString's improved move copy constructor as these methods return CStrings.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177066

[Inactive, 2014-06-26 15:08:28]

https://codereview.chromium.org/354023003/diff/1/Source/wtf/text/AtomicString.h
File Source/wtf/text/AtomicString.h (left):

https://codereview.chromium.org/354023003/diff/1/Source/wtf/text/AtomicString...
Source/wtf/text/AtomicString.h:77: AtomicString(const AtomicString& other) :
m_string(other.m_string) { }
Let me know if you'd prefer to specify explicitly that we're using the default
implementation for those (i.e. "* = default"). This is not required for this
class but I can understand we want to make this more explicit.

[tkent, 2014-06-26 23:20:17]

lgtm

[commit-bot: I haz the power, 2014-06-26 23:21:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/354023003/1

[Nico, 2014-06-26 23:36:09]

Sorry about forgetting to reply for this.

We don't do C++11 yet. We shouldn't add more support macros (like this cl does,
for defaulted functions).

Even once we support C++11, we won't allow all features probably. We need to
figure out which ones we want to use first.

[Nico, 2014-06-26 23:36:11]

The CQ bit was unchecked by thakis@chromium.org

[Inactive, 2014-06-27 00:17:16]

On 2014/06/26 23:36:09, Nico (away) wrote:
> Sorry about forgetting to reply for this.
> 
> We don't do C++11 yet. We shouldn't add more support macros (like this cl
does,
> for defaulted functions).

I understand we don't do C++11 by default yet but we are certainly going in this
direction. Also, you just enabled it by default for gcc 4.8 (and thus Android),
or was it reverted? In any case, it seems odd to have a macro for deleted
functions (and use them) but not have defaulted functions.

> Even once we support C++11, we won't allow all features probably. We need to
> figure out which ones we want to use first.

I understand we'll need to choose a set of C++11 feature we can use by default.
I still think it is unfortunate we cannot use other C++11 functionality, under
macro. Clang 3.0 and GCC 4.4 both support defaulted functions so a lot of
targets (including Android) could potentially leverage this. 

I'd like to move forward with this, so would the following be OK with you?
1. Leave the WTFString copy constructors / assignment operators explicitly
defined (to avoid using =default)
2. Still add the move constructor and assignment operator to RefPtr
3. Don't explicitly define the move constructor / assignment operator for
AtomicString (=default is not needed there)

[Nico, 2014-06-27 01:52:37]

On 2014/06/27 00:17:16, Chris Dumez wrote:
> On 2014/06/26 23:36:09, Nico (away) wrote:
> > Sorry about forgetting to reply for this.
> > 
> > We don't do C++11 yet. We shouldn't add more support macros (like this cl
> does,
> > for defaulted functions).
> 
> I understand we don't do C++11 by default yet but we are certainly going in
this
> direction.

We're going to have C++11 eventually, but it's not clear how much of it we're
going to use. Blink has some code for it that came in through webkit, and since
it's not clear where we're going to land I didn't remove what was there, but I
don't want to add to it for now.

> Also, you just enabled it by default for gcc 4.8 (and thus Android),
> or was it reverted?

It's still in.

> In any case, it seems odd to have a macro for deleted
> functions (and use them) but not have defaulted functions.

I agree!

> > Even once we support C++11, we won't allow all features probably. We need to
> > figure out which ones we want to use first.
> 
> I understand we'll need to choose a set of C++11 feature we can use by
default.
> I still think it is unfortunate we cannot use other C++11 functionality, under
> macro. Clang 3.0 and GCC 4.4 both support defaulted functions so a lot of
> targets (including Android) could potentially leverage this. 

GCC only supports it if you build with -std=c++11 though I think, and much of
the codebase doesn't build with -std=c++11 for 4.4 (at least it doesn't with
4.6; maybe 4.4 fares better since it implements fewer restrictions)?

> 1. Leave the WTFString copy constructors / assignment operators explicitly
> defined (to avoid using =default)
> 2. Still add the move constructor and assignment operator to RefPtr
> 3. Don't explicitly define the move constructor / assignment operator for
> AtomicString (=default is not needed there)

[Nico, 2014-06-27 01:56:20]

On 2014/06/27 00:17:16, Chris Dumez wrote:
> On 2014/06/26 23:36:09, Nico (away) wrote:
> > Sorry about forgetting to reply for this.
> > 
> > We don't do C++11 yet. We shouldn't add more support macros (like this cl
> does,
> > for defaulted functions).
> 
> I understand we don't do C++11 by default yet but we are certainly going in
this
> direction. Also, you just enabled it by default for gcc 4.8 (and thus
Android),
> or was it reverted? In any case, it seems odd to have a macro for deleted
> functions (and use them) but not have defaulted functions.
> 
> > Even once we support C++11, we won't allow all features probably. We need to
> > figure out which ones we want to use first.
> 
> I understand we'll need to choose a set of C++11 feature we can use by
default.
> I still think it is unfortunate we cannot use other C++11 functionality, under
> macro. Clang 3.0 and GCC 4.4 both support defaulted functions so a lot of
> targets (including Android) could potentially leverage this. 
> 
> I'd like to move forward with this, so would the following be OK with you?
> 1. Leave the WTFString copy constructors / assignment operators explicitly
> defined (to avoid using =default)
> 2. Still add the move constructor and assignment operator to RefPtr
> 3. Don't explicitly define the move constructor / assignment operator for
> AtomicString (=default is not needed there)

I missed replying to this, sorry. I'd prefer not adding more move constructors,
but since we already have a bunch I don't have a good argument against it. So
I'm fine with this plan.

[Inactive, 2014-06-27 02:04:31]

> > Also, you just enabled it by default for gcc 4.8 (and thus Android),
> > or was it reverted?
> 
> It's still in.

Great!

> > > Even once we support C++11, we won't allow all features probably. We need
to
> > > figure out which ones we want to use first.
> > 
> > I understand we'll need to choose a set of C++11 feature we can use by
> default.
> > I still think it is unfortunate we cannot use other C++11 functionality,
under
> > macro. Clang 3.0 and GCC 4.4 both support defaulted functions so a lot of
> > targets (including Android) could potentially leverage this. 
> 
> GCC only supports it if you build with -std=c++11 though I think, and much of
> the codebase doesn't build with -std=c++11 for 4.4 (at least it doesn't with
> 4.6; maybe 4.4 fares better since it implements fewer restrictions)?

Just reporting what's on https://gcc.gnu.org/projects/cxx0x.html
-std=c++0x is definitely needed (-std=c++11 is only available since gcc 4.7).

I am not surprised if the codebase is not building with older gcc and C++11
though (haven't tried). I am mostly interested in gcc 4.8 which is what is
available for Android nowadays (and also what I have on my Linux desktop).

> > 1. Leave the WTFString copy constructors / assignment operators explicitly
> > defined (to avoid using =default)
> > 2. Still add the move constructor and assignment operator to RefPtr
> > 3. Don't explicitly define the move constructor / assignment operator for
> > AtomicString (=default is not needed there)

You did not comment on my counter-proposal, I hope it addresses your concerns.

[Inactive, 2014-06-27 02:05:54]

On 2014/06/27 02:04:31, Chris Dumez wrote:
> > > Also, you just enabled it by default for gcc 4.8 (and thus Android),
> > > or was it reverted?
> > 
> > It's still in.
> 
> Great!
> 
> > > > Even once we support C++11, we won't allow all features probably. We
need
> to
> > > > figure out which ones we want to use first.
> > > 
> > > I understand we'll need to choose a set of C++11 feature we can use by
> > default.
> > > I still think it is unfortunate we cannot use other C++11 functionality,
> under
> > > macro. Clang 3.0 and GCC 4.4 both support defaulted functions so a lot of
> > > targets (including Android) could potentially leverage this. 
> > 
> > GCC only supports it if you build with -std=c++11 though I think, and much
of
> > the codebase doesn't build with -std=c++11 for 4.4 (at least it doesn't with
> > 4.6; maybe 4.4 fares better since it implements fewer restrictions)?
> 
> Just reporting what's on https://gcc.gnu.org/projects/cxx0x.html
> -std=c++0x is definitely needed (-std=c++11 is only available since gcc 4.7).
> 
> I am not surprised if the codebase is not building with older gcc and C++11
> though (haven't tried). I am mostly interested in gcc 4.8 which is what is
> available for Android nowadays (and also what I have on my Linux desktop).
> 
> > > 1. Leave the WTFString copy constructors / assignment operators explicitly
> > > defined (to avoid using =default)
> > > 2. Still add the move constructor and assignment operator to RefPtr
> > > 3. Don't explicitly define the move constructor / assignment operator for
> > > AtomicString (=default is not needed there)
> 
> You did not comment on my counter-proposal, I hope it addresses your concerns.

Oh you just did :)

[Inactive, 2014-06-27 03:21:31]

I updated the patch as per my counter-proposal. Please take another look.

[Nico, 2014-06-27 03:32:57]

lgtm, thanks!

[Inactive, 2014-06-27 03:48:51]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-06-27 03:51:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/354023003/20001

[commit-bot: I haz the power, 2014-06-27 04:54:13]

Change committed as 177066

[Mikhail, 2014-06-27 07:48:44]

https://codereview.chromium.org/354023003/diff/20001/Source/wtf/RefPtr.h
File Source/wtf/RefPtr.h (right):

https://codereview.chromium.org/354023003/diff/20001/Source/wtf/RefPtr.h#newc...
Source/wtf/RefPtr.h:114: swap(o);
This does not look correct, look
{
RefPtr<A> a = adoptRef(new A); // First object.
RefPtr<A> b = adoptRef(new A); // Second object.
a = std::move(b);
// We anticipate that the first object has gone but it's not the case
}

We need to have a temporary here.
Pls have a look at https://codereview.chromium.org/14031031

[Inactive, 2014-06-27 13:02:13]

On 2014/06/27 07:48:44, mikhail.pozdnyakov wrote:
> https://codereview.chromium.org/354023003/diff/20001/Source/wtf/RefPtr.h
> File Source/wtf/RefPtr.h (right):
> 
>
https://codereview.chromium.org/354023003/diff/20001/Source/wtf/RefPtr.h#newc...
> Source/wtf/RefPtr.h:114: swap(o);
> This does not look correct, look
> {
> RefPtr<A> a = adoptRef(new A); // First object.
> RefPtr<A> b = adoptRef(new A); // Second object.
> a = std::move(b);
> // We anticipate that the first object has gone but it's not the case
> }
> 
> We need to have a temporary here.
> Pls have a look at https://codereview.chromium.org/14031031

Thanks Mikhail. This is indeed unexpected behavior. I will take a look at this
today.