base: Add out-of-line copy ctors for complex classes.

base: Add out-of-line copy ctors for complex classes.

This patch adds out of line copy constructors for classes that our
clang-plugin considers heavy. This is an effort to enable copy
constructor checks by default.

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

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

[vmpstr, 2016-04-18 19:41:28]

Description was changed from

==========
base: : Add out-of-line copy ctors for complex classes.

This patch adds out of line copy constructors for classes that our
clang-plugin considers heavy. This is an effort to enable copy
constructor checks by default.

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

to

==========
base: Add out-of-line copy ctors for complex classes.

This patch adds out of line copy constructors for classes that our
clang-plugin considers heavy. This is an effort to enable copy
constructor checks by default.

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

[vmpstr, 2016-04-18 19:43:59]

Please take a look

[danakj, 2016-04-18 19:44:59]

danakj@chromium.org changed reviewers:
+ fdoray@chromium.org, gab@chromium.org, robliao@chromium.org

[danakj, 2016-04-18 19:45:00]

LGTM

[vmpstr, 2016-04-18 19:45:30]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2016-04-18 19:46:08]

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

[robliao, 2016-04-18 19:46:09]

lgtm

[commit-bot: I haz the power, 2016-04-18 21:05:14]

Description was changed from

==========
base: Add out-of-line copy ctors for complex classes.

This patch adds out of line copy constructors for classes that our
clang-plugin considers heavy. This is an effort to enable copy
constructor checks by default.

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

to

==========
base: Add out-of-line copy ctors for complex classes.

This patch adds out of line copy constructors for classes that our
clang-plugin considers heavy. This is an effort to enable copy
constructor checks by default.

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

[commit-bot: I haz the power, 2016-04-18 21:05:15]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-04-18 21:07:17]

Description was changed from

==========
base: Add out-of-line copy ctors for complex classes.

This patch adds out of line copy constructors for classes that our
clang-plugin considers heavy. This is an effort to enable copy
constructor checks by default.

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

to

==========
base: Add out-of-line copy ctors for complex classes.

This patch adds out of line copy constructors for classes that our
clang-plugin considers heavy. This is an effort to enable copy
constructor checks by default.

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

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

[commit-bot: I haz the power, 2016-04-18 21:07:18]

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

[gab, 2016-04-19 18:36:49]

Is this correct? I'm guessing this is only required for the STL, should we
provide a move-only constructor instead to be able to move the ref instead of
bumping it around?

[vmpstr, 2016-04-19 18:42:06]

On 2016/04/19 18:36:49, gab wrote:
> Is this correct? I'm guessing this is only required for the STL, should we
> provide a move-only constructor instead to be able to move the ref instead of
> bumping it around?

Explicit dtor prevents an implicit move ctor from being generated, so the code
was doing a copy. This patch doesn't change behavior, it just makes it explicit
that there is a copy ctor. That being said, if this could be a move ctor
instead, then we should definitely do that instead! :)

[gab, 2016-04-19 18:51:27]

On 2016/04/19 18:42:06, vmpstr wrote:
> On 2016/04/19 18:36:49, gab wrote:
> > Is this correct? I'm guessing this is only required for the STL, should we
> > provide a move-only constructor instead to be able to move the ref instead
of
> > bumping it around?
> 
> Explicit dtor prevents an implicit move ctor from being generated, so the code
> was doing a copy. This patch doesn't change behavior, it just makes it
explicit
> that there is a copy ctor. That being said, if this could be a move ctor
> instead, then we should definitely do that instead! :)

Right I understand you didn't change behaviour :-), my reply was mostly
inquiring other reviewers about what we could do better here. We discussed
something offline, I'll follow-up, thanks for pointing this out.

[danakj, 2016-04-19 18:56:49]

On Tue, Apr 19, 2016 at 11:36 AM, <gab@chromium.org> wrote:

> Is this correct? I'm guessing this is only required for the STL, should we
>

(Most?) STL containers do not do copies anymore. We're doing a copy
somewhere else. = delete the constructor to find out where :)


> provide a move-only constructor instead to be able to move the ref instead
> of
> bumping it around?
>
> https://codereview.chromium.org/1895903003/
>

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