Is =delete preferred over DISALLOW_COPY_AND_ASSIGN now?

[Ben Chan]

Hi,

I'm wondering what's the current recommendation regarding the use of DISALLOW_COPY_AND_ASSIGN vs =delete in new code.

The last post from Nico suggested continuing to use DISALLOW_COPY_AND_ASSIGN: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/RBnvY7Nms4k/4jSD4lrrBAAJ

The Google C++ style guide has since updated to mention only =delete (and go/cpp-style#Copyable_Movable_Types explicitly discourages DISALLOW_COPY_AND_ASSIGN for new code)

Thanks,

Ben

[Nico Weber]

http://chromium-cpp.appspot.com/ currently says "continue to use DISALLOW_COPY_AND_ASSIGN". But https://groups.google.com/a/chromium.org/forum/#!searchin/cxx/DISALLOW_COPY_AND_ASSIGN|sort:date/cxx/tJaJi9fhhLc/jCK4FJBUBgAJ and https://groups.google.com/a/chromium.org/forum/#!searchin/cxx/DISALLOW_COPY_AND_ASSIGN|sort:date/cxx/4dWUEOSyTxo/gkm_RrhVBgAJ sound like there's support for moving to = delete given that google style has changed, so I'd suggest preparing a change to styleguide/c++/c++11.html and sending a short rfc for that that to cxx@. I'd guess nobody would have a problem with moving to = delete at this point.

[Daniel Bratell]

There was also some discussion about DISALLOW_COPY_AND_ASSIGN not being right in a post-move world, so that is should be DISALLOW_COPY_AND_ASSIGN_AND_MOVE. Then it seems approximately better to have a macro than to rely on remembering to delete all ~3 generated methods.

/Daniel

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAMGbLiEEGrQLowJd2n_bA8gndb1ZjDi5aGKpTAzdsMR927EEeQ%40mail.gmail.com.

--

/* Opera Software, Linköping, Sweden: CEST (UTC+2) */

[Peter Kasting]

On Wed, Jun 6, 2018 at 7:06 PM Nico Weber <tha...@chromium.org> wrote:

I'd suggest preparing a change to styleguide/c++/c++11.html and sending a short rfc for that that to cxx@. I'd guess nobody would have a problem with moving to = delete at this point.

+1 -- the style guide is unambiguous now and we don't have a strong reason to differ.

It's also worth noting that the Google style guide is different than it was years ago in two other ways:

• There is some guidance that you need not mark subclasses as non-copyable when they inherit from a non-copyable class, unless the subclass itself is doing something that should make it obviously non-copyable even if the parent were to change.  The common case is "subclass just tweaks the behavior in some way that doesn't really affect copyability, so no need to re-specify."
• There is more leeway for making copyable/movable classes than before; rather than "make uncopyable unless you must copy", it's more "decide whether the class is more plausibly a copyable or uncopyable type and then be explicit".

It might be worth calling out these shifts in direction in some way to the team?

PK 

[Jeremy Roman]

I'd like to move to match upstream Google style. It would be a little nicer if someone could do a Chromium-wide change so that we're consistent everywhere, but it'd be slightly tricky to do.

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFC4SW7fS8%3Dde2h8gMqPntgFOzRKOFR3tpD9XtsHaJjwPg%40mail.gmail.com.

[Alexei Svitkine]

I believe last time we discussed it, there were a few cons to the "= delete" syntax that were brought up:

  - Ambiguity about where it should be in the C++ file (still last? at the top?). If no strong guidance, it means it will be inconsistent and reviewer has to do more work to see if there.

  - More lines of code thus more noise to read vs. a single macro people have to be aware of.

  - Inconsistency between existing code and new code if we don't do a rewrite of everything.

  - Easier to get wrong (i.e. the macro does the right thing - with the explicit spelling out, you can omit one of the constructors, etc).

For these reasons, I still think it's better to keep the macro.

The downside is we're inconsistent with Google style. The upside are all the reasons above, which I think outweigh the inconsistency. (And I speak as someone who also writes internal Google C++, so in theory the upside of switching should have more impact for me than for most Chromium developers who don't write C++ internally at Google).

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CACuR13d%2BHAwG%2BA0N5KbQsUMvDzx48Vssz4PfQAFdY4b-NPcdUg%40mail.gmail.com.

[Peter Kasting]

On Thu, Jun 7, 2018 at 11:38 AM Alexei Svitkine <asvi...@chromium.org> wrote:

I believe last time we discussed it, there were a few cons to the "= delete" syntax that were brought up:

  - Ambiguity about where it should be in the C++ file (still last? at the top?). If no strong guidance, it means it will be inconsistent and reviewer has to do more work to see if there.

The style guide has an example which places this where you would expect constructors/assignment operators to be declared.  I've been told examples in the style guide are normative unless otherwise noted, and this placement makes sense with the rest of the guide.  So I'd count this as "guidance to place in the public constructor/assignment operator section".

 

  - More lines of code thus more noise to read vs. a single macro people have to be aware of.

  - Inconsistency between existing code and new code if we don't do a rewrite of everything.

  - Easier to get wrong (i.e. the macro does the right thing - with the explicit spelling out, you can omit one of the constructors, etc).

I think these objections are fair.  But I also think they apply equally to Google internally, and if we want to make different decision than they did, we need to understand their line of reasoning sufficiently to see why Chromium has different constraints.  As you noted, you write code internally, so I'm curious if you have a take on this.

You have not mentioned reasons why this is a good change:

• Consistency with modern C++ coding conventions elsewhere
• Use of standard language idioms that lowers the barrier to codebase entry for external people
• Copyability/non-copyability is now consistently declared in the same place in the class every time
• Non-copyability becomes part of the public API of the class (seems clearer)
• IMO, prods devs to think about copyability and movability distinctly, which is probably a good consideration when designing a class

Some of these overlap or are trivial, but I do think the benefits outweigh the costs here.  I do think it's reasonable to disagree :).  But I also want to limit the number of exceptions we make to the Google style guide, so the question is not just whether this is a good case for an exception, but whether this is one of the best cases to spend our (imaginary) "exceptions budget" on.

PK

[Reilly Grant]

Resending from the correct address.

This recently came up again on a review: https://chromium-review.googlesource.com/c/chromium/src/+/1528751/5/device/fido/win/webauthn_api_adapter.h#143

If we are deprecating use of DISALLOW_COPY_AND_ASSIGN we should update macros.h with a deprecation notice. I personally prefer the macro because it is easier to understand the intent but I will do whatever the style guide tells me to because the style guide is always correct.

I have created a CL adding the deprecation notice: https://chromium-review.googlesource.com/c/chromium/src/+/1694142

Reilly Grant | Software Engineer | rei...@chromium.org | Google Chrome

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFAay9zJfZHmEk8h_qbC8_VajhCE0SQEOEAJQzrP%3DF6UJA%40mail.gmail.com.

[K Moon]

Ping? Recently ran into the same issue. The public C++ style guide seems to endorse the "= delete" syntax, but doesn't forbid DISALLOW_COPY_AND_ASSIGN (which it doesn't know about, of course, since DISALLOW_COPY_AND_ASSIGN is part of base), and there's no deprecation notice on the macro. It "feels" like "= delete" is the right thing to do, but it's fuzzy enough to be confusing.

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

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAEmk%3DMYru8PmvkgoMpxbU9ErtnY4W8E1DmcywTux0%2BAvBgwWaA%40mail.gmail.com.

[K Moon]

+cxx

[dan...@chromium.org]

On Wed, Jul 31, 2019 at 8:48 PM K Moon <km...@chromium.org> wrote:

+cxx

On Wed, Jul 31, 2019 at 5:48 PM K Moon <km...@chromium.org> wrote:

Ping? Recently ran into the same issue. The public C++ style guide seems to endorse the "= delete" syntax, but doesn't forbid DISALLOW_COPY_AND_ASSIGN (which it doesn't know about, of course, since DISALLOW_COPY_AND_ASSIGN is part of base), and there's no deprecation notice on the macro. It "feels" like "= delete" is the right thing to do, but it's fuzzy enough to be confusing.

My take is that I believe the macro is useful and common enough that we should continue to use it unless we're going to rewrite everything to =delete instead. Since that is surely not worth anyone's time, I would say we should just keep using the macro. We could add it to documentation to make that more clear.

 

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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-4n_r6pX48zc_YRaKz0xzzk_ee4y6zUXOUz4BWzSybcaw%40mail.gmail.com.

[Alexei Svitkine]

I share danakj's opinion. Besides consistency, it's also more concise (1 line instead of 2) - and easier to get right (i.e. requires less time for author and CL reviewer since the statement is simpler).

[Gabriel Charette]

On Fri, Aug 2, 2019 at 11:09 AM Alexei Svitkine <asvi...@chromium.org> wrote:

I share danakj's opinion. Besides consistency, it's also more concise (1 line instead of 2) - and easier to get right (i.e. requires less time for author and CL reviewer since the statement is simpler).

On Fri, Aug 2, 2019 at 11:06 AM <dan...@chromium.org> wrote:

On Wed, Jul 31, 2019 at 8:48 PM K Moon <km...@chromium.org> wrote:

+cxx

On Wed, Jul 31, 2019 at 5:48 PM K Moon <km...@chromium.org> wrote:

Ping? Recently ran into the same issue. The public C++ style guide seems to endorse the "= delete" syntax, but doesn't forbid DISALLOW_COPY_AND_ASSIGN (which it doesn't know about, of course, since DISALLOW_COPY_AND_ASSIGN is part of base), and there's no deprecation notice on the macro. It "feels" like "= delete" is the right thing to do, but it's fuzzy enough to be confusing.

My take is that I believe the macro is useful and common enough that we should continue to use it unless we're going to rewrite everything to =delete instead. Since that is surely not worth anyone's time, I would say we should just keep using the macro. We could add it to documentation to make that more clear.

+1

 

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

[Scott Violet]

+1. I'm pretty sure there is another thread on this as well, which I've advocated for only changing to =delete if there are folks actively working on converting all existing code.

  -Scott

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7LLapscOYbkBXxYLAZG_P8ep6vFSALNT4FvokcmEZLUS%2Bg%40mail.gmail.com.

[K Moon]

I don't think the difficulty of a mass conversion to =delete should be the primary reason why DISALLOW_COPY_AND_ASSIGN should stay, as it's a very mechanical, local transformation that I suspect could be automated easily by a single person. I would think most of the work would be getting approvals for code reviews, which could be done incrementally. (If I had to hazard a completely unscientific guess, I don't think it would take more than a few days to a week.)

If we keep DISALLOW_COPY_AND_ASSIGN, I do think we should put an exception in the Chromium C++ style guide, as I think it's enough in conflict with the Google C++ style guide to require some sort of explicit decision/clarification.

The relevant section from the Google C++ style guide (https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types) reads:

"Decision: Every class's public interface should make explicit which copy and move operations the class supports. This should usually take the form of explicitly declaring and/or deleting the appropriate operations in the public section of the declaration.

Specifically, a copyable class should explicitly declare the copy operations, a move-only class should explicitly declare the move operations, and a non-copyable/movable class should explicitly delete the copy operations. Explicitly declaring or deleting all four copy/move operations is permitted, but not required. If you provide a copy or move assignment operator, you must also provide the corresponding constructor."

While it's debatable whether DISALLOW_COPY_AND_ASSIGN counts as "explicitly" deleting the unwanted operations, I think the common practice of putting DISALLOW_COPY_AND_ASSIGN in the private: section clearly is in conflict. The practice should be at least to put DISALLOW_COPY_AND_ASSIGN in the public: section (presumably near the constructors), or add an explicit exemption to the Chromium C++ style guide to the contrary.

As for whether or not DISALLOW_COPY_AND_ASSIGN is a good thing to have, from what I've been able to gather from crawling old discussion threads, I think the main arguments against it boil down to:

1. It's not part of the language like =delete is (and arguably is less idiomatic and transparent as a result).

2. It's only a partial solution, since similar macros don't exist for disallowing move as well.

3. The syntax better parallels the case in which you do declare the copy/move operations.

4. You need to include and depend on a (small) additional header to use it.

5. It's a macro, and we don't like macros.

[K Moon]

(Peter's original reply also has some points in favor of getting rid of DISALLOW_COPY_AND_ASSIGN as well.)

[Yuri Wiitala]

When discussing other issues, we adhere to the philosophy of "there should be one way to do it, even if there are multiple just-as-good ways of doing it" to keep things clean and consistent. Ex: scoped_refptr versus std::shared_ptr, base::Time[Ticks] versus <chrono>, base::OnceCallback versus std::packaged_task/std::function, etc.

Based on pkasting's notes about the changes in the Google C++ style guide (and that we have no good reason to override it for Chromium's needs), it seems it's clear that we should deprecate/ban DISALLOW_COPY_AND_ASSIGN() and spawn a code maintenance project to replace all existing ones with =delete.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-7Su7MHerXCHCm07i3wTDmhXqU7PFaQnvAojZq3q5CtuQ%40mail.gmail.com.

[Peter Kasting]

On Fri, Aug 2, 2019 at 3:05 PM Yuri Wiitala <m...@chromium.org> wrote:

Based on pkasting's notes about the changes in the Google C++ style guide (and that we have no good reason to override it for Chromium's needs), it seems it's clear that we should deprecate/ban DISALLOW_COPY_AND_ASSIGN() and spawn a code maintenance project to replace all existing ones with =delete.

Right now that maintenance project is on my list of things to do "someday".  It will require someone writing a custom clang-format pass to do this conversion and then manually reviewing at least a large subset of the results.  IMO, we should combine this with a pass to remove DISALLOW_COPY from classes whose parent is already non-copyable (per the style guide, such classes do not need to explicitly ban copying) as this will dramatically reduce the number of places that need to be migrated.

In the meantime, I have been advising people to use =delete in new code.

PK

[Reilly Grant]

In the interest of not recommending two ways to do something, if we agree that recommending =delete in new code is the right thing, then let's add a note to base/macros.h reflecting that. Otherwise we shouldn't be advising people not to follow the example demonstrated by //base.

Reilly Grant | Software Engineer | rei...@chromium.org | Google Chrome

[Peter Kasting]

On Tue, Aug 13, 2019 at 1:03 PM Reilly Grant <rei...@chromium.org> wrote:

In the interest of not recommending two ways to do something, if we agree that recommending =delete in new code is the right thing, then let's add a note to base/macros.h reflecting that.

I have no opposition to that.

PK 

[K Moon]

That would make me very happy. :-)

[Alexei Svitkine]

However, from the thread, we didn't reach an agreement?

I'm fine changing my stance if I'm the only one that prefers the macros. But it seems that I'm not the only one?

Should we do a survey of committers to see what option the majority prefer?

[Darin Fisher]

It seems like the point of the style guide is to make it so we don't have to have a lot of discussion and debate on coding style? The point is that we should allow ourselves to strongly err on the side of just doing whatever the style guide says and calling it a day. SG?

Personally, I prefer the macro for all the reasons stated in this thread, but I even more so prefer not debating coding style! ;-)

WDYT? Accept the style guide and call it a day?

-Darin

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKFU2SDdNRXq9ddeGU5MK9JXqeqR4Lk1aud_U%3Dmqs8gYSKGbvQ%40mail.gmail.com.

[Peter Kasting]

On Tue, Aug 13, 2019 at 2:22 PM Alexei Svitkine <asvi...@chromium.org> wrote:

However, from the thread, we didn't reach an agreement?

The most recent activity on this topic is summarized here: https://groups.google.com/a/chromium.org/d/msg/cxx/qwH2hxaEjac/TUKq6eqfCwAJ

To quote: "Summary: It seems like there's strong consensus to use the internal Google style direction [i.e., =delete]."  The vote was something like 9-0 in favor.

PK

[Alexei Svitkine]

Thanks for the context for the cxx mailing list discussion.

I was referring to the emails on this thread where Dana, Gab and Scott (and I) were in favor of the macro.

Since there was overwhelming support on the cxx thread, it sounds like those favoring the macro are in the minority, so I take back my personal push back.