Proposal: Deprecate DISALLOW_COPY_AND_ASSIGN

[Peter Kasting]

Proposed: Mark DISALLOW_COPY_AND_ASSIGN deprecated via a comment (possibly also a PRESUBMIT).  Remove reference to using this from the C++11/14 features page.  Notify chromium-dev that this is deprecated in favor of explicit =delete.

The Google style guide no longer refers to this macro, and it is discouraged internally.

The most recent discussion on this was at https://groups.google.com/a/chromium.org/d/topic/chromium-dev/p0hd3qUE9J0/discussion and didn't really conclude.  There was explicit support from me and jbroman, and (unclear) support from thakis.  There was opposition from asvitkine.  bratell talked about remembering to delete all generated methods, but as the style guide notes, only deleting the copy methods is necessary; the move methods are implicitly deleted when the copy methods are deleted.

This came up this week on my subteam, so I'd like to formally resolve one way or the other.

PK

[Ryan Hamilton]

I'm very supportive of removing this and using C++ native features instead. How hard would it be to actually remove this from the codebase?

I'm not in love with deprecations unless we're actively removing existing users. Otherwise we end up with situations where refactoring results in violations of the deprecation policy.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFBja0yvfRQTPDo%2Bhuvd7uHr%3DDFZ5SDcmWfV9_h0zevngw%40mail.gmail.com.

[Ken Rockot]

On Fri, Mar 8, 2019 at 2:11 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

When it's come up in the past IIRC many people complained that it would be significantly more verbose than the macro. I still like the proposal, because you could make the same argument for tons of other verbose C++ patterns that we don't have macros to compress. If we had =delete a long time ago, the macro probably would have never been added.

Anyway, if we did this could we have the style plugin enforce that you must explicitly either delete, default, or define a copy ctor and =? Would that be too annoying for things like simple structs?

[Chris Blume]

I support this.

A blind replace of DISALLOW_COPY_AND_ASSIGN() with the =delete methods is unlikely to be completely sufficient. This is typically the last thing in a class and thus likely to be private.

There might be some fancy clang tooling way to do it. But it would need to be smart. We can't just blindly make them public, either. When the information was lost by making DISALLOW_COPY_AND_ASSIGN() private we lost the ability to easily recreate what it should have been.

But that said, I still support the idea.

Chris Blume | Software Engineer | cbl...@google.com | +1-614-929-9221

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJ_4DfRGfqeLOPoU4KziZ5iZ8pr%2Beu%3DgLgX5FPALE_Hv%3DjwzJQ%40mail.gmail.com.

[Peter Kasting]

On Fri, Mar 8, 2019 at 2:18 PM Ryan Hamilton <r...@chromium.org> wrote:

I'm very supportive of removing this and using C++ native features instead. How hard would it be to actually remove this from the codebase?

Don't know.  A find-and-replace could do it, but we'd probably also want to move the deletions up to the constructors part of the public section, and I don't know how to do that automatically.  I suppose we could argue that latter is unnecessary, but then existing code sort of encourages people to delete these at the end of their private section, which seems regrettable.

I'm not in love with deprecations unless we're actively removing existing users. Otherwise we end up with situations where refactoring results in violations of the deprecation policy.

Yes, I'm not a big fan of "unfunded mandate" things either.  In this case, I can't volunteer to take on the gruntwork task of global switchover :(.  If someone else wants to...

PK

[Peter Kasting]

On Fri, Mar 8, 2019 at 2:24 PM Chris Blume <cbl...@google.com> wrote:

A blind replace of DISALLOW_COPY_AND_ASSIGN() with the =delete methods is unlikely to be completely sufficient. This is typically the last thing in a class and thus likely to be private.

There might be some fancy clang tooling way to do it. But it would need to be smart. We can't just blindly make them public, either. When the information was lost by making DISALLOW_COPY_AND_ASSIGN() private we lost the ability to easily recreate what it should have been.

I'm not exactly sure what you're saying will break if this conversion is done.  The macro is implemented using =delete already (I made that change several years ago), so find-and-replace with =delete will have zero functional effect.

And if the functions are deleted, it's not clear to me that there's any functional difference around how their access control is declared.  I think this is only semantic clarity?  Personally I'm OK with them being public-in-all-cases, but maybe there's an obvious counterexample.  For example, this seems OK to me:

// BEFORE:

class Foo {

 protected:

  Foo();

 private:

  DISALLOW_COPY_AND_ASSIGN(Foo);

};

// AFTER:

class Foo {

 public:

  Foo(const Foo&) = delete;

  Foo& operator=(const Foo&) = delete;

 protected:

  Foo();

};

PK

[Chris Blume]

Oh, nothing will break. Sorry.

In all cases it can't be called.

The only nit-picky glasses-lift "well actually" thing is whether the error says "cannot call private function" or "method was deleted".

I wouldn't consider that to be anywhere close to a show stopper.

[Peter Kasting]

On Fri, Mar 8, 2019 at 2:43 PM Chris Blume <cbl...@google.com> wrote:

The only nit-picky glasses-lift "well actually" thing is whether the error says "cannot call private function" or "method was deleted".

I believe (but am not certain) that when we changed to using =delete in the implementation, the error message switched to "method was deleted" in all cases, so even the error would not change.

PK 

[Chris Blume]

Even better. :)

I rescind my comment. It is so insignificant that I don't think we should bother caring.

[Giovanni Ortuño]

FWIW, I appreciated the macro when I first joined Chrome and started learning C++. I was not familiar with move-only types and the macro made very clear what was happening.

Do we know  if new team members look at the C++ in Chromium Codelab or any other common C++ resource where we could add a short summary or pointer to more information about move-only types?

Gio

--

[Peter Kasting]

On Sun, Mar 10, 2019 at 3:41 PM Giovanni Ortuño <ort...@chromium.org> wrote:

FWIW, I appreciated the macro when I first joined Chrome and started learning C++. I was not familiar with move-only types and the macro made very clear what was happening.

I'm a bit confused.  DISALLOW_COPY_AND_ASSIGN isn't really related to move-only types (indeed, it predates them).  It's shorthand for making a type non-copyable, but by default that also makes the type non-movable as well.  Declaring a move-only type requires explicitly declaring the move constructors in addition to deleting the copy ones.

Do we know  if new team members look at the C++ in Chromium Codelab or any other common C++ resource where we could add a short summary or pointer to more information about move-only types?

The Google style guide speaks at some length about move-only types and shows how to declare them.  If people aren't reading the Google style guide I'm a bit worried about whether any other place will be as effective :).

PK

[lo...@chromium.org]

Hi, all!

+1 for removing this macro.

In many cases we will be able to remove one extra line:
#include "base/macros.h"

WBR, Alexey.

[Giovanni Ortuño]

On Mon, Mar 11, 2019, 3:20 PM Peter Kasting <pkas...@google.com wrote:

On Sun, Mar 10, 2019 at 3:41 PM Giovanni Ortuño <ort...@chromium.org> wrote:

FWIW, I appreciated the macro when I first joined Chrome and started learning C++. I was not familiar with move-only types and the macro made very clear what was happening.

I'm a bit confused.  DISALLOW_COPY_AND_ASSIGN isn't really related to move-only types (indeed, it predates them).  It's shorthand for making a type non-copyable, but by default that also makes the type non-movable as well.  Declaring a move-only type requires explicitly declaring the move constructors in addition to deleting the copy ones.

Ah, sorry. I meant non-copiable. 

Do we know  if new team members look at the C++ in Chromium Codelab or any other common C++ resource where we could add a short summary or pointer to more information about move-only types?

The Google style guide speaks at some length about move-only types and shows how to declare them.  If people aren't reading the Google style guide I'm a bit worried about whether any other place will be as effective :).

PK

Ah, I wasn't aware that this was mentioned in the style guide. I can't think of any other more effective places to mention this.

Thanks,

Gio

[Natalie Chouinard]

In my very first CL, I added a DISALLOW_COPY_AND_ASSIGN without really understanding it. As I learned more C++ and read the Google style guide, I tried to remove it in favour of explicit = delete: https://chromium-review.googlesource.com/c/chromium/src/+/1478174/4/chrome/browser/ui/webui/feed_internals/feed_internals_page_handler.h#24

That was as good a time as any to learn about the difference between Chromium style guide and Google style guide, but I think the fewer special cases we have the better. I'd generally prefer being explicit over macros, especially for new people.

Natalie

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAMDKGBW1gL4XAQ0ERtMpj9Ko05rEK%3D1%3DfbER1pmqaw7BfJ7UPg%40mail.gmail.com.

[Sylvain Defresne]

I am in favor of removing the macro and converting the code to use = delete. I don't think there is need to deviate from internal style guide here.

-- Sylvain

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CA%2BV2g111QZZOTA_EW1i4ZXgyGt42aDeMjy3SezrQN-KZFs4vjw%40mail.gmail.com.

[Jan Wilken Dörrie]

+1 to deprecating / deleting the macro and migrating code. It's probably helpful to include a link to https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types in the PRESUBMIT check in case we introduce one.

Furthermore, we likely should also deprecate and remove the other related macros, i.e. DISALLOW_COPY, DISALLOW_ASSIGN and DISALLOW_IMPLICIT_CONSTRUCTORS.

Cheers,

Jan

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJauWCFUQAi9WPD4uORxLHkL_s%3DRqDULLN2LuuU9xP9sETJYPA%40mail.gmail.com.

Jan Wilken Dörrie

Software Engineer

jdoe...@google.com
 +49 89 839300973

Google Germany GmbH

Erika-Mann-Straße 33

80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Falls sie diese fälschlicherweise erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich bitte wissen, dass die E-Mail an die falsche Person gesendet wurde.

    

This e-mail is confidential. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it has gone to the wrong person.

[Jeremy Roman]

Agreed that it would be nice to be rid of these but I don't consider it a very high priority.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CALB5StbfeH87-5hXaTBavSp8vkkeM8uONTGeNdUy7V6drQ6-CQ%40mail.gmail.com.

[dan...@chromium.org]

I'd like to replace the macro, but I am not a fan of marking it as deprecated. That will only increase the variety of ways we see classes marked non-copyable.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACuR13cvegcLYoKAeTdWpnGiNp%2B9dqAmympwWMLWYxZwr7f8Lg%40mail.gmail.com.

[Peter Kasting]

Summary: It seems like there's strong consensus to use the internal Google style direction.

A couple people (including me) would like to do a full conversion instead of leaving us in an inconsistent state.  vmpstr@ just sent me a CL to convert all of cc/, so I'm hoping there's an automated way to do everything (including moving to the public section).  If so, I think the plan should be "add notes to guides as necessary, announce to chromium-dev, convert the whole codebase, remove the existing macros".

I will try to find a way forward here.

PK

[dan...@chromium.org]

Thanks Peter and Vlad.

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFDEgdS_FaKsNVsUjRycJ2r6B1DdG88Uua0KmOrV%2Bv5ENQ%40mail.gmail.com.

[Peter Kasting]

On Tue, Mar 12, 2019 at 11:32 AM Peter Kasting <pkas...@google.com> wrote:

A couple people (including me) would like to do a full conversion instead of leaving us in an inconsistent state.  vmpstr@ just sent me a CL to convert all of cc/, so I'm hoping there's an automated way to do everything (including moving to the public section).  If so, I think the plan should be "add notes to guides as necessary, announce to chromium-dev, convert the whole codebase, remove the existing macros".

It turns out the cc/ conversion had a large amount of hand-done work.  We can either convert-in-place or someone more capable than me can whip up a script or clang-based tool to try and do the "move to public section" part.  Please speak up if you're willing to help with the latter.

PK 

[s...@google.com]

A strong +1 to *only* allowing the new style if we have a plan and people actively working on removing the old. Our code base has become more inconsistent of late. We should actively work on consistency as it helps reduce friction when working in different parts of the code base.

  -Scott