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.
- 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/3256969Nov 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