DISALLOW_COPY_AND_ASSIGN and subclasses of non copyable/assignable classes?

[Matthew Dempsky]

If a parent class uses DISALLOW_COPY_AND_ASSIGN, should/must subclasses use it too?  Does it matter if the subclass would otherwise be okay to copy/assign if the base classes were too?  Or if they have internal linkage and/or are only used in unit test code?

This came up in crrev.com/289193003 where I converted a bunch of our sandbox testing policies to inherit from SandboxBPFPolicy, which is a DISALLOW_COPY_AND_ASSIGN base class.  For conciseness/simplicity(/laziness...) I didn't bother adding DISALLOW_COPY_AND_ASSIGN in the new test policy classes.  There was an initial concern that this might be undefined behavior in C++03, but further discussion suggests it's actually defined, just of uncertain style.

Our current thought is since these are internal test classes and its defined behavior, it's probably not worth adding the DISALLOW_COPY_AND_ASSIGN statements.  But if they were a public API, we'd probably add them if only to document the behavior so people don't need to additionally inspect the base classes.

What say you, chromium-dev?

[Ryan Sleevi]

I generally insist that subclasses have it too, when reviewing.

This makes it easy to look at a glance whether the author intended the default operators, without having to manually inspect the inheritance tree to see if C++ is going to implicitly prohibit it.

The Style Guide doesn't list exceptions for this case, which is another argument in favour of always adding it.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Rachel Blum]

+1 for DISALLOW

It should only be left out if there's a compelling reason to actually make the class copyable.

 - Rachel

[Julien Tinnes]

If a class is exposed in a header file, DISALLOW_COPY_AND_ASSIGN() makes it more readable.

In this very specific case of several inline class definition in a unit test file, I think it would make sense to omit it. Omitting in this case is what helps readability.

Julien

[Ryan Sleevi]

While I appreciate the position, you can still end up hiding bugs - which explicit DISALLOW helps.

A common (slightly unfortunate) pattern is to friend some helper class, which serves as the test base, in order to expose certain members for the test to mutate.

However, doing so also exposes the ability for the derived to make implicit copies of the parent.

DISALLOW_COPY_AND_ASSIGN is certainly not limited in utility to header files. It forces it to be a fast failure when you do something you shouldn't be - and helps to document your expectations.

Readability is important, but part of readability is being able to look at a bit of code and correctly reason about its effects. This is why we encourage documentation such as interfaces being implemented, why annotations like OVERRIDE are useful (beyond the static verification aspect), and arguably, why to document whether or not something is intentionally copyable.

[Matthew Dempsky]

On Mon, May 19, 2014 at 3:40 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

A common (slightly unfortunate) pattern is to friend some helper class, which serves as the test base, in order to expose certain members for the test to mutate.

However, doing so also exposes the ability for the derived to make implicit copies of the parent.

But unless you use DISALLOW_COPY_AND_ASSIGN() and then still provide a copy constructor / assignment operator, you'll end up with a link error.  It won't just end up falling back to calling the implicit ones anyway.

However, it's not entirely unreasonable that someone might *misuse* the DISALLOW_* macros *and* accidentally friend some test code that then invokes them.  We can prevent that by changing the DISALLOW_* macros to use "= delete" when available; I've filed crbug.com/375000 to track that.  It seems a bit painful, but not impossible at the moment.

[Peter Kasting]

On Mon, May 19, 2014 at 2:53 PM, Matthew Dempsky <mdem...@chromium.org> wrote:

If a parent class uses DISALLOW_COPY_AND_ASSIGN, should/must subclasses use it too?

The Google style guide says that unless your class needs to be copyable, it must have DISALLOW_COPY_AND_ASSIGN.  That means child classes, internal classes, unit test classes, and everything else.

If some of your classes are missing them, it's not the end of the world, but they should have them, and if you are touching the files anyway, please do add them.

PK

[Matthew Dempsky]

On Mon, May 19, 2014 at 5:04 PM, Peter Kasting <pkas...@chromium.org> wrote:

The Google style guide says that unless your class needs to be copyable, it must have DISALLOW_COPY_AND_ASSIGN.  That means child classes, internal classes, unit test classes, and everything else.

Boo. :(  Done.