Semi-automatic tool to replace DISALLOW_COPY_AND_ASSIGN

[Samuel Huang]

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]

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]

Sorry I missed the first manual parts. Ignore the latter.

[Peter Boström]

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]

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.

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

[Peter Boström]

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]

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

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:

https://pkg.go.dev/go.timothygu.me/tools/cr/cmd/remove_disallow

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]

(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]

I also made a tool a while ago to do this:

https://pkg.go.dev/go.timothygu.me/tools/cr/cmd/remove_disallow

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.

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.

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

[Lei Zhang]

FYI, as of https://crrev.com/938828 which landed last week, all the
DISALLOW_* macros have been removed from base/macros.h. \o/

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

[K. Moon]

Great work! Super excited to see this land. Would love a retrospective from pbos@ about how much work this was.

[Peter Boström]

I think I can put some notes down here at least. crbug.com/1010217 has been closed as a 2.27x centithread which maybe means that we didn't fully appreciate the scale of this work when we started.

Timeline??

Mar 8, 2019 +Peter Kasting posts Proposal: Deprecate DISALLOW_COPY_AND_ASSIGN to cxx@. Consensus is eventually reached to only do so if there is a migration to the new style (explicit deletes).

* Note: The cost of doing this migration as far as I'm aware is never ball-parked. This should probably have happened at some point.

Oct 1, 2019 I file crbug.com/1010217 to start doing some migration work. pkasting@ almost immediately recommends a clang-tidy pass, I end up ignoring this over time. My bad. In my defense it was really hard to know how to get started with a one-off tool like this.

Nov 1, 2019 crrev.com/c/1895857 deprecates DISALLOW_ macros in dos-and-donts.

- Some combination of the above / possible email list discussion sets of a ton of small-scale work.

- Lots of CLs with ~10-50 files changed. This is essentially the first 147 comments.

Sep 15, 2021 This thread starts.

- Several attempts exist to make tools to automate away this problem. Afaik(?) none deal with inserting the deleted constructors in the correct place. At least none of them manage to automate the problem fully.

- I pile onto the box of tools with a Python script. Parsing C++ in python, how hard can it be? (If your tool is bad then your reviewers need to be good. +the...@chromium.org and +Daniel Cheng who did the vast majority of reviewing did good reviews. Thank you for not neglecting proper reviews here, you did more than your fair share.)

- kmoon@ probably has the best path to an actual tool (because it uses clang tooling rather than try to build a half-assed parser), but it seems unlikely to finish. Also unclear how well this would deal with preprocessor defines?

- I am overconfident in my tool and find that it can reduce 23950 instances to 7185, so I take crbug.com/1010217 again to try to make a dent in the problem.

Sep 16-Sep 27 (comment 147-185) are shards of this script. This is manually done roughly by top-level directory (unless that is too large) because git cl split is known to generate commits that are too small (never verified by me). A lot of these have merge conflicts between lgtm and submit so they should have been smaller.

Sep 28-Nov 4 roughly (comment 186-208) are manual removes of DISALLOW_ macros. Some of this is delayed by Perf, and it's not 100% focused on.

* This is done using vim with macros like :'<,'>s/ *DISALLOW_COPY_AND_ASSIGN(\(.*\));/\1(const \1\&) = delete;\r\1\& operator=(const \1\&) = delete;/.

* Clean up the private: section if DISALLOW_* was the only entry.

* shift-V to select the inserted rows, X to copy I believe, use % to go to the matching parenthesis (start of class), find last constructor instance in public section, P to insert.

* Above means, if you have to do something a few thousand times, make sure you know all of the ways your editor can make that faster. You can get pretty good at using the wrong tool for the job.

* Above also really means: This should've been a clang tool and not done manually. Knowledge of the above doesn't transfer to the next problem, but clang tooling could.

Some of the above includes removal of the macros themselves, which discovered that libassistant depended on src/base in a way that broke when removing DISALLOW_IMPLICIT_CONSTRUCTORS (and got the CL to remove it reverted). dcheng@ fixed this in crrev.com/c/3256969

Nov 5 (comment 209) submits crrev.com/c/3261552 which removes DISALLOW_* macros, effectively ending this task. Rest is cleanup.

Nov 8 adds #include "base/macros.h" to all files that reference ignore_result(), in order to make removal of base/macros.h easier (IWYU should be correct for ignore_result() at this point). This missed some non-c++ files that generate C++, which eventually had to be fixed.

Nov 10-12 (comment 212-221) removes #include "base/macros.h" from ~15k files I believe which I feel pays for all IWYU violations I've missed in the past.

Nov 12 lands crrev.com/c/3279210 which renames "macros.h" to "ignore_result.h" reflecting the only (non-macro) thing remaining.

Remaining time (until ~Nov 16) lands the corresponding macros.h -> ignore_result.h change in crashpad/mini_chromium and rolls both into chromium and unblocks future crashpad rolling.

Open questions / thoughts:

* PRESUBMIT.py warnings essentially shard work onto developers in an inefficient way. Neither the developer nor the reviewer has enough context to make an efficient dent in a problem of this magnitude.

  * 1000x developers doing 10k tryjobs of 1-10 fixes each has to be the least efficient way to solve anything like this. Also very likely to end up inconsistent.

  * Should there be some threshold for what goes in here? If it's in PRESUBMIT.py it's worth doing => it fits with code-health rotation? If it's not worth doing it as part of code-health rotation, is it really worth sharding on all developers?

  * Maybe PRESUBMIT.py could count violations, so if you move a violation that removes one and re-adds it, which has a net-neutral error count. Think android size bot.

  * LSC work often blocked on unrelated presubmit *errors*, pre-existing submit errors should be bot-breaking?

* Really hard to get into clang-tidy tools?

  * Seems to be a large knowledge gap in how to write clang-tidy tools for a lot of folks interested in doing LSC changes (or we probably wouldn't have a zillion C++-unaware scripts to solve this). This is probably worth investing in, maybe we can have a list of youtube talks/docs/getting started to help us get started here?

* Reviewers for churny changes

  * Underappreciated, hard to get reviewers without feeling like you're wasting their time, unless it's people who are already overburdened by reviews and interested in LSC changes. Often you are wasting reviewer time by sharding work onto too many reviewers (they don't have context).

  * Could reviewing be part of code-health rotation? Maybe you can do this rotation with a buddy and you can both review each other's changes, perhaps with help from the project proposer to make sure everyone knows how to review the proposed fixes.

  * When doing something like the above, you're only half the solution. Reviewers will have to review removal of instances in tens of thousands of files combined when the fixes are non-trivially rubber-stamped, and their work often goes unnoticed and underappreciated. Take this into account when considering this work. +the...@chromium.org repeatedly told me it was fine, I'm not sure if he was being nice. Peer bonuses can't make up for the work done here, make sure it's worth their time (and that they think it's worth their time).

* Was this worth it?

  * Honestly, I have no idea how to gauge if this was worth my time / my reviewers' time? It seems appreciated but it also took a long time and required reviewer heroics. Any help with how to value this change vs other code-health work that could've been done in these ~2 months would be appreciated. I do not know how to think of this and would personally like to have some better tools for evaluating what's worth doing/not doing. (:

  * Part of why this seemed worth it was that people kept trying to bite off smaller chunks of the large problem, and sharding seemed inefficient. If something like this wasn't worth our time, should it not have gone into PRESUBMIT.py? How can we discourage reviewers from asking for drive-by fixes to something we don't think is worth globally solving?

Those are some thoughts that I've had. Happy for any ideas and thoughts that you have as well.

Thanks all,

Peter

[Colin Blundell]

The above are really fantastic observations, Peter. I think that it would be well worth writing them up into a doc and starting a conversation about whether we should refine/revise our approach to this kind of issue.

Best,

Colin

 

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