Re: [chromium-dev] Re: Is =delete preferred over DISALLOW_COPY_AND_ASSIGN now?

[K Moon]

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

On Tue, Jul 9, 2019 at 4:51 PM Reilly Grant <rei...@chromium.org> wrote:

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

On Thu, Jun 7, 2018 at 1:01 PM Peter Kasting <pkas...@chromium.org> wrote:

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

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

--
--
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/CAEmk%3DMYru8PmvkgoMpxbU9ErtnY4W8E1DmcywTux0%2BAvBgwWaA%40mail.gmail.com.

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