Semi-automatic tool to replace DISALLOW_COPY_AND_ASSIGN

50 views
Skip to first unread message

Samuel Huang

unread,
Sep 15, 2021, 12:32:29 PMSep 15
to chromium-dev
Hi Everyone,

I grew tired of replacing multiple DISALLOW_COPY_AND_ASSIGN manually, so I wrote a simple tool to help (distributed as Chrome Gerrit copypasta instead of committing). Perhaps there are already plugins or better scripts, which will this look foolish? Please do share! Anyway:

Example:

$ cd ~/chrome/src/foo
$ cat foo.h

======== foo.h ========
#include "base/macros.h"
   
class Foo {
 public:
  Foo();
  ~Foo();
 protected:
  struct BarWithVeryLongName {
    BarWithVeryLongName();
   private:
    DISALLOW_COPY_AND_ASSIGN(
        BarWithVeryLongName);
  };

 private:
  bool foo_state_;
   
  DISALLOW_COPY_AND_ASSIGN(Foo);
};
========

Step 1: Manual edit:
  • Delete fluff (#include, unused private:, spaces.
  • Make DisALLOW_COPY_AND_ASSIGN into single lines.
  • Add MOVEHERE markers where boilerplate goes.

======== foo.h ========
class Foo {
 public:
  Foo();
  ~Foo();
  MOVEHERE
 protected:
  struct BarWithVeryLongName {
    BarWithVeryLongName();
    MOVEHERE_naive_string_search_matches_and_will_delete_line
    DISALLOW_COPY_AND_ASSIGN(BarWithVeryLongName);
  };

 private:
  bool foo_state_;
  DISALLOW_COPY_AND_ASSIGN(Foo);
};
========

Step 2: Dry-run script:
$ python3 ~/chrome/src/tools/refactor/replace_deaa.py foo.h

(output)
foo.h has 2 changes
========================================
Check complete: Rerun with -w switch to write!

Step 3: Perform actual write:
$ python3 ~/chrome/src/tools/refactor/replace_deaa.py -w foo.h

======== foo.h ========
class Foo {
 public:
  Foo();
  ~Foo();
Foo(const Foo&) = delete;
const Foo& operator=(const Foo&) = delete;
 protected:
  struct BarWithVeryLongName {
    
BarWithVeryLongName();
BarWithVeryLongName(const BarWithVeryLongName&) = delete;
const 
BarWithVeryLongName& operator=(const BarWithVeryLongName&) = delete;
  };

 private:
  bool foo_state_;
};
========

Step 4: Fix indents by running `git cl format`.

--
Samuel Huang

Peter Boström

unread,
Sep 15, 2021, 12:35:23 PMSep 15
to hua...@chromium.org, chromium-dev
Nit: MOVEHERE should go between Foo() and ~Foo(). Otherwise I like this, could this reasonably run on most of src with a LSC approver?

--
--
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/CAMve_yvM%3D_%3DUq%3DcXPd-zPjPia8CqTmyq5Cm3u-%3DTY_Cp_vtrJg%40mail.gmail.com.

Peter Boström

unread,
Sep 15, 2021, 12:36:52 PMSep 15
to hua...@chromium.org, chromium-dev
Sorry I missed the first manual parts. Ignore the latter.

Peter Boström

unread,
Sep 15, 2021, 6:16:56 PMSep 15
to Samuel Huang, chromium-dev
Up top so this gets more focus: I'd pretty strongly recommend grepping a directory for how many DISALLOW_COPY_AND_ASSIGNs are present before doing any manual work on this gargantuan task (knocking 100 off of 26000 is pretty drop-in-the-bucket unless everyone does it, which is probably not a great use of everyone's time).


In case you're interested, I put together my own version of a script with a couple of deficiencies. The code is hacky and only good enough to run not really to understand, and seems to not have a massive bug-to-usefulness ratio. crrev.com/c/3163571

It works as follows:

Identify classes that're subject to replace DISALLOW_COPY_AND_ASSIGN(Foo) by regex (regex is not a great C++ parser ofc)

For each of those go through the file and see if you can find (virtual) ~Foo( in the public section of a class. If you can, assume that inserting deleted constructors before ~Foo() is the right place.

Remove DISALLOW_COPY_AND_ASSIGN(Foo) and any comments/empty lines/private: label that directly preceded it.

Rinse and repeat. This is pretty hacky, but basically every time something gets replaced, restart. This may fail with DISALLOW_COPY_AND_ASSIGN still present because it doesn't know where to insert its replacement.


Results:

Two example runs are crrev.com/c/3163384 and crrev.com/c/3163108 and it seems to replace about half the instances (haven't looked into the others).

Bugs (found by code review) are at least (and counting), so please code review both yourself and w/ reviewer:
* If ~Foo() is under an #if the deleted constructors will be inserted there as well which is not intentional.
* If multiple classes in the same file have the same name (perhaps under different namespaces) this script gets confused and inserts a replacement once but removes all DISALLOW_COPY_AND_ASSIGNs for that name.

Also this does not IWYU, which is left as a separate pass (perhaps after these macros are gone).

If you can make this script deal with some remaining instances in base/ after crrev.com/c/3163384 lands that'd be cool, I don't think this DISALLOW_COPY_AND_ASSIGN thing should be resolved by manual inspection unless the manual time is waaaaaay shorter than it seems to currently be, so beware of being sniped into "knocking a few off the list" as there are many many many more where they came from (unless you find it meditative, which in that case go ahead). :)

Thanks, and thanks for keeping an eye out for code health as well.
Peter

On Wed, Sep 15, 2021 at 9:35 AM Samuel Huang <hua...@google.com> wrote:
Ah thanks for the correction. This is placed by manual edit though; you're given the freedom to make mistakes!

K. Moon

unread,
Sep 15, 2021, 6:44:21 PMSep 15
to Peter Boström, Samuel Huang, chromium-dev
I started working on a fully automated approach back in April, when I had some free time. (Said free time evaporated, so I haven't been able to get back to it.)

The remaining implementation is non-trivial, but I got far enough along that it seemed feasible. (There's just a decent amount of work to handle the edge and corner cases.) I uploaded my code so far to crrev.com/c/3163427, if anyone is curious and wants to take a look. But as I said, there's a fair amount of work still to get it to even fix simple cases.

Peter Boström

unread,
Sep 15, 2021, 7:16:03 PMSep 15
to K. Moon, Samuel Huang, chromium-dev
It looks like my script is able to reduce 23950 instances to 7185. That's a significant review load (and probably contains bugs), but I suggest we try to get this in scripted before doing any more manual work. I've taken crbug.com/1010217 for now.

After that I'm hoping that the rest will be mostly addressable by "find the last public constructor and insert there", very much not as convinced about my regex skills for C++ parsing but it may work OK and is hopefully worth doing.

If we're lucky then after that this is within manual-work swinging distance, if we are then we hopefully don't have to figure out the hard-correct-path route.

Best,
Peter

K. Moon

unread,
Sep 16, 2021, 10:31:49 AMSep 16
to Peter Boström, Samuel Huang, chromium-dev
+1. I think my tool is in a pretty good state to detect (but not fix) cases, so it might be useful to adapt it to spitting out a list of candidates, and cross-checking at some point.

K. Moon

unread,
Sep 17, 2021, 12:05:43 PMSep 17
to Timothy Gu, Chromium-dev, Samuel Huang, Peter Boström
Neat! The tool I was working on would "look through" any preprocessor definition that looked like it only deleted constructors, though, so it would work with arbitrarily-named and nested macros. (Or at least I think it did; I forget how far I got on that now.)

On Thu, Sep 16, 2021 at 9:09 PM Timothy Gu <timo...@chromium.org> wrote:
I also made a tool a while ago to do this:


One thing that's different compared to other tools here is that it deals with macros other than just DISALLOW_COPY_AND_ASSIGN:
  • DISALLOW_COPY
  • DISALLOW_ASSIGN
  • DISALLOW_IMPLICIT_CONSTRUCTORS
It also tries to be intelligent with regards to placing the deleted constructors.

Some instructions on how to use it are on the crbug itself (https://crbug.com/1010217#c92), but the gist is

go install go.timothygu.me/tools/cr/cmd/remove_disallow@latest
find src/your/directory -name '*.cc' -o -name '*.h' | xargs remove_disallow


P.S., there's also a companion tool remove_base_macros that removes any now-unneeded #include <base/macros.h>, which is useful after running remove_disallow.

Best,

Timothy

K. Moon

unread,
Sep 17, 2021, 12:24:49 PMSep 17
to Timothy Gu, Chromium-dev, Samuel Huang, Peter Boström
(Just to clarify, I don't think there were that many variations that I encountered, and a tool that works is infinitely preferable to a theoretically correct tool that doesn't. :-P)

Timothy Gu

unread,
Sep 20, 2021, 1:25:58 AMSep 20
to Chromium-dev, K. Moon, Samuel Huang, chromium-dev, Peter Boström
I also made a tool a while ago to do this:


One thing that's different compared to other tools here is that it deals with macros other than just DISALLOW_COPY_AND_ASSIGN:
  • DISALLOW_COPY
  • DISALLOW_ASSIGN
  • DISALLOW_IMPLICIT_CONSTRUCTORS
It also tries to be intelligent with regards to placing the deleted constructors.

Some instructions on how to use it are on the crbug itself (https://crbug.com/1010217#c92), but the gist is

go install go.timothygu.me/tools/cr/cmd/remove_disallow@latest
find src/your/directory -name '*.cc' -o -name '*.h' | xargs remove_disallow


P.S., there's also a companion tool remove_base_macros that removes any now-unneeded #include <base/macros.h>, which is useful after running remove_disallow.

Best,

Timothy
On Thursday, September 16, 2021 at 7:31:49 AM UTC-7 K. Moon wrote:
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

--
--
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+unsubscribe@chromium.org.
Reply all
Reply to author
Forward
0 new messages