Remove user-defined move constructor and assignment operator from WTF::String

Remove user-defined move constructor and assignment operator from WTF::String

WTF::String can rely on implicit move constructor and assignment
operator as WTF::RefPtr class has them defined now (r177066).

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

[Mikhail, 2014-07-03 13:26:26]

PTAL

[eseidel, 2014-07-03 17:37:51]

jyasskin is your best reviewer for c++ thneeds.  But LGTM.

[Mikhail, 2014-07-21 07:52:51]

The CQ bit was checked by mikhail.pozdnyakov@intel.com

[commit-bot: I haz the power, 2014-07-21 07:53:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/367343003/1

[commit-bot: I haz the power, 2014-07-21 08:30:21]

Change committed as 178541

[Inactive, 2014-07-21 11:15:43]

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

https://codereview.chromium.org/367343003/diff/1/Source/wtf/text/WTFString.h#...
Source/wtf/text/WTFString.h:132: String& operator=(String&& other) { m_impl =
other.m_impl.release(); return *this; }
Mikhail, are you sure about this? I thought the move constructor was not
generated here because of the user defined destructor.

[Inactive, 2014-07-21 12:32:25]

On 2014/07/21 11:15:43, Chris Dumez wrote:
> https://codereview.chromium.org/367343003/diff/1/Source/wtf/text/WTFString.h
> File Source/wtf/text/WTFString.h (left):
> 
>
https://codereview.chromium.org/367343003/diff/1/Source/wtf/text/WTFString.h#...
> Source/wtf/text/WTFString.h:132: String& operator=(String&& other) { m_impl =
> other.m_impl.release(); return *this; }
> Mikhail, are you sure about this? I thought the move constructor was not
> generated here because of the user defined destructor.

As I suspected (confirmed with gcc 4.8 and C++11 enabled):
Your patch increased the binary size by 12Kb (63158456 to 63170744 on the
stripped content_shell release binary). The generated assembly code is larger,
see diff for WTFString here:
http://pastebin.com/JNKu7u2z

I think we should revert this change.

[Mikhail, 2014-07-21 12:35:44]

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

https://codereview.chromium.org/367343003/diff/1/Source/wtf/text/WTFString.h#...
Source/wtf/text/WTFString.h:132: String& operator=(String&& other) { m_impl =
other.m_impl.release(); return *this; }
On 2014/07/21 11:15:43, Chris Dumez wrote:
> Mikhail, are you sure about this? I thought the move constructor was not
> generated here because of the user defined destructor. 
Thanks for catching this, Chris! The thing is that WTF::String does not need to
have user-defined destructor, I should have removed it here. I'll remove it with
the following CL.

[Inactive, 2014-07-21 12:36:25]

A working solution would have been to bring back the default implementation
using "= default;" instead. However, I already proposed this in an earlier CL
and Nico rejected it because we don't want to rely on all C++11 features just
yet.

We could also try to remove the explicitly defined destructor (it is there only
to make sure it is always inlined). However, iirc, I already tried that and the
resulting code was different so this likely has performance implications.

[Inactive, 2014-07-21 12:37:56]

On 2014/07/21 12:35:44, Mikhail wrote:
> https://codereview.chromium.org/367343003/diff/1/Source/wtf/text/WTFString.h
> File Source/wtf/text/WTFString.h (left):
> 
>
https://codereview.chromium.org/367343003/diff/1/Source/wtf/text/WTFString.h#...
> Source/wtf/text/WTFString.h:132: String& operator=(String&& other) { m_impl =
> other.m_impl.release(); return *this; }
> On 2014/07/21 11:15:43, Chris Dumez wrote:
> > Mikhail, are you sure about this? I thought the move constructor was not
> > generated here because of the user defined destructor. 
> Thanks for catching this, Chris! The thing is that WTF::String does not need
to
> have user-defined destructor, I should have removed it here. I'll remove it
with
> the following CL.

Ok, please be careful about doing this as I already tried (see comment below).

Maybe we would revert and then propose it as a new patch? It would make
reverting cleaner if it turns out to cause perf regressions.

[Mikhail, 2014-07-21 12:55:01]

On 2014/07/21 12:37:56, Chris Dumez wrote:
> On 2014/07/21 12:35:44, Mikhail wrote:
> > https://codereview.chromium.org/367343003/diff/1/Source/wtf/text/WTFString.h
> > File Source/wtf/text/WTFString.h (left):
> > 
> >
>
https://codereview.chromium.org/367343003/diff/1/Source/wtf/text/WTFString.h#...
> > Source/wtf/text/WTFString.h:132: String& operator=(String&& other) { m_impl
=
> > other.m_impl.release(); return *this; }
> > On 2014/07/21 11:15:43, Chris Dumez wrote:
> > > Mikhail, are you sure about this? I thought the move constructor was not
> > > generated here because of the user defined destructor. 
> > Thanks for catching this, Chris! The thing is that WTF::String does not need
> to
> > have user-defined destructor, I should have removed it here. I'll remove it
> with
> > the following CL.
> 
> Ok, please be careful about doing this as I already tried (see comment below).
> 
> Maybe we would revert and then propose it as a new patch? It would make
> reverting cleaner if it turns out to cause perf regressions.

Ok, let's revert it for now, then I'll try to remove user-defined dtor and if
there are no regressions (I do not believe they turn up) we can apply this patch
again.

[Mikhail, 2014-07-21 12:56:11]

A revert of this CL has been created in
https://codereview.chromium.org/402163002/ by mikhail.pozdnyakov@intel.com.

The reason for reverting is: This can be applied only when user-defined dtor is
removed..