Add more out of line copy ctors for complex classes.

Add more out of line copy ctors for complex classes.

This patch adds the remaining copy constructors for complex classes.
After this patch, it should be possible to enable the heavy class copy
constructor checks by default.

R=thakis@chromium.org, dcheng@chromium.org
TBR=jam@chromium.org
BUG=436357
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[vmpstr, 2016-03-22 23:16:22]

Description was changed from

==========
Add more out of line copy ctors for complex classes.

This patch adds the remaining copy constructors for complex classes.
After this patch, it should be possible to enable the heavy class copy
constructor checks by default.

R=thakis@chromium.org, dcheng@chromium.org
BUG=436357
==========

to

==========
Add more out of line copy ctors for complex classes.

This patch adds the remaining copy constructors for complex classes.
After this patch, it should be possible to enable the heavy class copy
constructor checks by default.

R=thakis@chromium.org, dcheng@chromium.org
BUG=436357
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[vmpstr, 2016-03-22 23:16:23]

Description was changed from

==========
Add more out of line copy ctors for complex classes.

This patch adds the remaining copy constructors for complex classes.
After this patch, it should be possible to enable the heavy class copy
constructor checks by default.

R=thakis@chromium.org, dcheng@chromium.org
BUG=436357
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add more out of line copy ctors for complex classes.

This patch adds the remaining copy constructors for complex classes.
After this patch, it should be possible to enable the heavy class copy
constructor checks by default.

R=thakis@chromium.org, dcheng@chromium.org
BUG=436357
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[vmpstr, 2016-03-22 23:16:24]

Description was changed from

==========
Add more out of line copy ctors for complex classes.

This patch adds the remaining copy constructors for complex classes.
After this patch, it should be possible to enable the heavy class copy
constructor checks by default.

R=thakis@chromium.org, dcheng@chromium.org
BUG=436357
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add more out of line copy ctors for complex classes.

This patch adds the remaining copy constructors for complex classes.
After this patch, it should be possible to enable the heavy class copy
constructor checks by default.

R=thakis@chromium.org, dcheng@chromium.org
BUG=436357
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[vmpstr, 2016-03-22 23:22:43]

Please take a look.

https://codereview.chromium.org/1825273002/diff/20001/content/public/common/f...
File content/public/common/frame_navigate_params.h (right):

https://codereview.chromium.org/1825273002/diff/20001/content/public/common/f...
content/public/common/frame_navigate_params.h:23: struct CONTENT_EXPORT
FrameNavigateParams : public IPC::NoParams {
Note that this is required, because there is an IPC struct that derives from
content::FrameNagivateParams and then is copied. The clang plugin check skips
any struct that is derived (directly or indirectly) from IPC::NoParams. This
seemed like the easiest way to fix the issue.

Let me know if you have a different solution in mind.

[dcheng, 2016-03-22 23:25:06]

LGTM

Now for copy operators and move constructors/operators =P

https://codereview.chromium.org/1825273002/diff/20001/content/public/common/f...
File content/public/common/frame_navigate_params.h (right):

https://codereview.chromium.org/1825273002/diff/20001/content/public/common/f...
content/public/common/frame_navigate_params.h:23: struct CONTENT_EXPORT
FrameNavigateParams : public IPC::NoParams {
On 2016/03/22 at 23:22:43, vmpstr wrote:
> Note that this is required, because there is an IPC struct that derives from
content::FrameNagivateParams and then is copied. The clang plugin check skips
any struct that is derived (directly or indirectly) from IPC::NoParams. This
seemed like the easiest way to fix the issue.
> 
> Let me know if you have a different solution in mind.

It's definitely kind of a hack, but I think it's OK. The alternatives seem
strictly worse.

[no sievers, 2016-03-22 23:37:19]

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

[no sievers, 2016-03-22 23:37:20]

lgtm

https://codereview.chromium.org/1825273002/diff/20001/content/public/common/f...
File content/public/common/frame_navigate_params.h (right):

https://codereview.chromium.org/1825273002/diff/20001/content/public/common/f...
content/public/common/frame_navigate_params.h:23: struct CONTENT_EXPORT
FrameNavigateParams : public IPC::NoParams {
On 2016/03/22 23:25:06, dcheng wrote:
> On 2016/03/22 at 23:22:43, vmpstr wrote:
> > Note that this is required, because there is an IPC struct that derives from
> content::FrameNagivateParams and then is copied. The clang plugin check skips
> any struct that is derived (directly or indirectly) from IPC::NoParams. This
> seemed like the easiest way to fix the issue.
> > 
> > Let me know if you have a different solution in mind.
> 
> It's definitely kind of a hack, but I think it's OK. The alternatives seem
> strictly worse.

I'd put a comment though.

[vmpstr, 2016-03-22 23:57:43]

Going to TBR other owners

https://codereview.chromium.org/1825273002/diff/20001/content/public/common/f...
File content/public/common/frame_navigate_params.h (right):

https://codereview.chromium.org/1825273002/diff/20001/content/public/common/f...
content/public/common/frame_navigate_params.h:23: struct CONTENT_EXPORT
FrameNavigateParams : public IPC::NoParams {
On 2016/03/22 23:37:20, sievers wrote:
> On 2016/03/22 23:25:06, dcheng wrote:
> > On 2016/03/22 at 23:22:43, vmpstr wrote:
> > > Note that this is required, because there is an IPC struct that derives
from
> > content::FrameNagivateParams and then is copied. The clang plugin check
skips
> > any struct that is derived (directly or indirectly) from IPC::NoParams. This
> > seemed like the easiest way to fix the issue.
> > > 
> > > Let me know if you have a different solution in mind.
> > 
> > It's definitely kind of a hack, but I think it's OK. The alternatives seem
> > strictly worse.
> 
> I'd put a comment though.

Done.

[vmpstr, 2016-03-22 23:58:38]

Description was changed from

==========
Add more out of line copy ctors for complex classes.

This patch adds the remaining copy constructors for complex classes.
After this patch, it should be possible to enable the heavy class copy
constructor checks by default.

R=thakis@chromium.org, dcheng@chromium.org
BUG=436357
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Add more out of line copy ctors for complex classes.

This patch adds the remaining copy constructors for complex classes.
After this patch, it should be possible to enable the heavy class copy
constructor checks by default.

R=thakis@chromium.org, dcheng@chromium.org
TBR=jam@chromium.org
BUG=436357
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[vmpstr, 2016-03-23 00:01:30]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2016-03-23 00:01:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
sievers@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1825273002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-03-23 00:01:59]

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

[vmpstr, 2016-03-23 00:24:43]

On 2016/03/22 23:25:06, dcheng wrote:
> LGTM
> 
> Now for copy operators and move constructors/operators =P

I think msvc2015 is sticking around. Now that it's there we can probably do move
ctors too, since we can = default them :D I think it's definitely worth while
doing this, but maybe we need some sort of a macro like
COPYMOVE_DEFINITION(Foo); COPYMOVE_DECLARATION(Foo); that will do both copy/move
ctors/assignment operators. I'm also not very good with names, so COPYMOVE can
probably be named something sane instead. 

> 
>
https://codereview.chromium.org/1825273002/diff/20001/content/public/common/f...
> File content/public/common/frame_navigate_params.h (right):
> 
>
https://codereview.chromium.org/1825273002/diff/20001/content/public/common/f...
> content/public/common/frame_navigate_params.h:23: struct CONTENT_EXPORT
> FrameNavigateParams : public IPC::NoParams {
> On 2016/03/22 at 23:22:43, vmpstr wrote:
> > Note that this is required, because there is an IPC struct that derives from
> content::FrameNagivateParams and then is copied. The clang plugin check skips
> any struct that is derived (directly or indirectly) from IPC::NoParams. This
> seemed like the easiest way to fix the issue.
> > 
> > Let me know if you have a different solution in mind.
> 
> It's definitely kind of a hack, but I think it's OK. The alternatives seem
> strictly worse.

[commit-bot: I haz the power, 2016-03-23 00:26:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-23 00:26:10]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[vmpstr, 2016-03-23 01:05:12]

On 2016/03/23 00:26:10, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
> (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

It seems that gcc has a bug when it comes to dealing with out of line ctors of
unions. For example,

$ cat test.cpp 
union Foo {
  Foo(const Foo& other);

  int a;
  float b;
};

Foo::Foo(const Foo& other) = default;

$ g++ -std=c++11 -c test.cpp
test.cpp: In copy constructor ‘Foo::Foo(const Foo&)’:
test.cpp:1:7: error: initializations for multiple members of ‘Foo’
 union Foo {
       ^
test.cpp: At global scope:
test.cpp:8:30: note: synthesized method ‘Foo::Foo(const Foo&)’ first required
here 
 Foo::Foo(const Foo& other) = default;
                              ^


clang doesn't have a problem with this. Also, if the ctor is = default but
inside the body of the union, the gcc is fine with it as well. I'll update the
code not to use = default with that one particular union.

[vmpstr, 2016-03-23 01:09:06]

vmpstr@chromium.org changed reviewers:
+ estade@chromium.org

[vmpstr, 2016-03-23 01:09:07]

+estade. Could you please take a look at ui/native_theme? Thanks!

https://codereview.chromium.org/1825273002/diff/60001/ui/native_theme/native_...
File ui/native_theme/native_theme.cc (right):

https://codereview.chromium.org/1825273002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme.cc:19: memcpy(this, &other, sizeof(*this));
This seems to be on par with what the default ctor is doing.

[danakj, 2016-03-23 01:21:40]

On Tue, Mar 22, 2016 at 5:24 PM, <vmpstr@chromium.org> wrote:

> On 2016/03/22 23:25:06, dcheng wrote:
> > LGTM
> >
> > Now for copy operators and move constructors/operators =P
>
> I think msvc2015 is sticking around. Now that it's there we can probably
> do move
>

There's some beta blocker for it that if not resolved will cause it to be
reverted in order to ship the next beta, so it's not clear yet..


> ctors too, since we can = default them :D I think it's definitely worth
> while
> doing this, but maybe we need some sort of a macro like
> COPYMOVE_DEFINITION(Foo); COPYMOVE_DECLARATION(Foo); that will do both
> copy/move
> ctors/assignment operators. I'm also not very good with names, so COPYMOVE
> can
> probably be named something sane instead.
>
> >
> >
>
>
https://codereview.chromium.org/1825273002/diff/20001/content/public/common/f...
> > File content/public/common/frame_navigate_params.h (right):
> >
> >
>
>
https://codereview.chromium.org/1825273002/diff/20001/content/public/common/f...
> > content/public/common/frame_navigate_params.h:23: struct CONTENT_EXPORT
> > FrameNavigateParams : public IPC::NoParams {
> > On 2016/03/22 at 23:22:43, vmpstr wrote:
> > > Note that this is required, because there is an IPC struct that
> derives from
> > content::FrameNagivateParams and then is copied. The clang plugin check
> skips
> > any struct that is derived (directly or indirectly) from IPC::NoParams.
> This
> > seemed like the easiest way to fix the issue.
> > >
> > > Let me know if you have a different solution in mind.
> >
> > It's definitely kind of a hack, but I think it's OK. The alternatives
> seem
> > strictly worse.
>
>
>
> https://codereview.chromium.org/1825273002/
>

-- 
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.

[vmpstr, 2016-03-24 18:28:14]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2016-03-24 18:28:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
sievers@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1825273002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2016-03-24 18:28:39]

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

[commit-bot: I haz the power, 2016-03-24 20:23:02]

Description was changed from

==========
Add more out of line copy ctors for complex classes.

This patch adds the remaining copy constructors for complex classes.
After this patch, it should be possible to enable the heavy class copy
constructor checks by default.

R=thakis@chromium.org, dcheng@chromium.org
TBR=jam@chromium.org
BUG=436357
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Add more out of line copy ctors for complex classes.

This patch adds the remaining copy constructors for complex classes.
After this patch, it should be possible to enable the heavy class copy
constructor checks by default.

R=thakis@chromium.org, dcheng@chromium.org
TBR=jam@chromium.org
BUG=436357
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-03-24 20:23:04]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-24 20:25:10]

Description was changed from

==========
Add more out of line copy ctors for complex classes.

This patch adds the remaining copy constructors for complex classes.
After this patch, it should be possible to enable the heavy class copy
constructor checks by default.

R=thakis@chromium.org, dcheng@chromium.org
TBR=jam@chromium.org
BUG=436357
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Add more out of line copy ctors for complex classes.

This patch adds the remaining copy constructors for complex classes.
After this patch, it should be possible to enable the heavy class copy
constructor checks by default.

R=thakis@chromium.org, dcheng@chromium.org
TBR=jam@chromium.org
BUG=436357
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[commit-bot: I haz the power, 2016-03-24 20:25:12]

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