Fix clang plugin to catch defaulted inlined ctors/dtors.

Fix clang plugin to catch defaulted inlined ctors/dtors.

BUG=459607, 436357
Committed: https://crrev.com/1acec8bccdfc3de4679a572e107767188ec31609
Cr-Commit-Position: refs/heads/master@{#317413}

[dcheng, 2015-02-18 20:38:45]

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

[dcheng, 2015-02-18 20:38:46]

So...

The good news is this is easy to fix.
The bad news is it's going to require a massive cleanup effort in the codebase
(I can't even build base_unittests successfully right now).
The good news is I could probably jury rig it to ignore copy constructors and
use the legacy checking behavior for them (by far, this seems to be the biggest
pain point).
The bad news is this would make the checking logic even more complicated and
harder to read, making it harder to maintain.

Thoughts? =)

(Also, maybe we should just encourage people to use {} for defaulted
constructors/destructors?)

[Nico, 2015-02-18 20:51:15]

lgtm

How massive is massive? = default hasn't been allowed for long, I'd be surprised
if it's heavily used.

Personally, {} seems much better to me than = default for structors.

[dcheng, 2015-02-18 20:54:51]

On 2015/02/18 at 20:51:15, thakis wrote:
> lgtm
> 
> How massive is massive? = default hasn't been allowed for long, I'd be
surprised if it's heavily used.
> 
> Personally, {} seems much better to me than = default for structors.

The problem is the copy constructor check is getting randomly skipped at the
moment:
../../base/memory/weak_ptr.h:87:3: error: [chromium-style] Complex class/struct
needs an explicit out-of-line copy constructor.
  class BASE_EXPORT Flag : public RefCountedThreadSafe<Flag> {
  ^
../../base/memory/weak_ptr.h:83:1: error: [chromium-style] Complex class/struct
needs an explicit out-of-line copy constructor.
class BASE_EXPORT WeakReference {
^
../../base/memory/weak_ptr.h:113:1: error: [chromium-style] Complex class/struct
needs an explicit out-of-line copy constructor.
class BASE_EXPORT WeakReferenceOwner {
^
In file included from ../../base/async_socket_io_handler_posix.cc:5:
In file included from ../../base/async_socket_io_handler.h:9:
In file included from ../../base/sync_socket.h:21:
../../base/synchronization/waitable_event.h:150:3: error: [chromium-style]
Complex class/struct needs an explicit out-of-line copy constructor.
  struct WaitableEventKernel :
  ^

And that's just from one file.

[dcheng, 2015-02-19 18:01:01]

New patchsets have been uploaded after l-g-t-m from thakis@chromium.org

[dcheng, 2015-02-19 18:02:16]

Here's a slightly more complicated version that should avoid breaking backwards
compatibility with Chromium (outside of = default).

[dcheng, 2015-02-20 18:21:50]

ping!

now comes with bonus fixes in chromium.

[Nico, 2015-02-20 18:39:05]

this lgtm too. Should we add a flag to enable the "real" check while here?

[dcheng, 2015-02-20 19:11:23]

On 2015/02/20 at 18:39:05, thakis wrote:
> this lgtm too. Should we add a flag to enable the "real" check while here?

What's a "real" check? A version of this check that wouldn't accidentally skip
copy constructors in the instances highlighted in the updated tests?

[Nico, 2015-02-20 19:15:03]

yes, your ps1

On Fri, Feb 20, 2015 at 11:11 AM, <dcheng@chromium.org> wrote:

> On 2015/02/20 at 18:39:05, thakis wrote:
>
>> this lgtm too. Should we add a flag to enable the "real" check while here?
>>
>
> What's a "real" check? A version of this check that wouldn't accidentally
> skip
> copy constructors in the instances highlighted in the updated tests?
>
> https://codereview.chromium.org/937833003/
>

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

[dcheng, 2015-02-20 19:58:04]

On 2015/02/20 at 19:15:03, thakis wrote:
> yes, your ps1
> 
> On Fri, Feb 20, 2015 at 11:11 AM, <dcheng@chromium.org> wrote:
> 
> > On 2015/02/20 at 18:39:05, thakis wrote:
> >
> >> this lgtm too. Should we add a flag to enable the "real" check while here?
> >>
> >
> > What's a "real" check? A version of this check that wouldn't accidentally
> > skip
> > copy constructors in the instances highlighted in the updated tests?
> >
> > https://codereview.chromium.org/937833003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
> 

I'd like to pursue that in followup patches, since I think there are several
issues to address:

- Copy constructors: the fact that the check is omitting them seems to be
clearly accidental rather than intended. This should be fixed.
- Copy assignment: omitted originally by erg@ due to the reasons mentioned in
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/LWTe_z4kmxo...
(tl;dr no =default made copy/assignment operators easy to mess up, but this is
moot with C++11 feature support)
- Move constructors: I don't think anyone has measured the binary cost of move
constructors yet.
- Move assignment: I suspect we'd want to do whatever we do for move
constructors here.

Does that sound OK? We can use the bug I originally filed about the plugin
randomly not catching some instances.

[Nico, 2015-02-20 20:01:17]

sounds good!

[dcheng, 2015-02-20 21:03:01]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2015-02-20 21:03:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/937833003/60001

[commit-bot: I haz the power, 2015-02-20 21:59:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-20 22:00:34]

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