refactor: Remove MakeUnique<T>() #21404
Conversation
|
Seems fine, but let's wait for the list of conflicts. Two minor style nits (feel free to ignore):
|
There was a problem hiding this comment.
Concept ACK, although I agree with Marco that we should wait for drahtbot to tell us how many PRs this conflicts with, and hold back if any of those are important and close to being merged.
Will this silently conflict with PRs that introduce MakeUnique() calls? Is there a way to find those and force a CI rerun?
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ACK: nice cleanup! |
-BEGIN VERIFY SCRIPT- git rm src/util/memory.h sed -i -e 's/MakeUnique/std::make_unique/g' $(git grep -l MakeUnique src) sed -i -e '/#include <util\/memory.h>/d' $(git grep -l '#include <util/memory.h>' src) sed -i -e '/util\/memory.h \\/d' src/Makefile.am -END VERIFY SCRIPT-
271cc06 to
1a6323b
Compare
|
Looks like the list of potential conflicts is fairly small, only 13. With some of those already being rebased regularly, and some based on other PRs, i.e #21206. I've taken the |
|
utACK 1a6323b Change looks correct. No comment on when this should be merged. I'll defer to the wisdom of the maintainers 🧙 🧙 🧙 |
|
cr ACK 1a6323b: patch looks correct |
There was a problem hiding this comment.
ACK 1a6323b looks correct
Will this silently conflict with PRs that introduce MakeUnique() calls? Is there a way to find those and force a CI rerun?
Sounds like fanquake has been 👀ing the MakeUniques for a while. But generally, it sounds like we have a lot of things we want to get rid of. If we want to soak the bandaid a little first, could write a linter for them, add the linter to CI and give it some time to catch new occurrences, then land the removal PR. Otherwise you basically have to flush the pipeline, either by searching for the silent conflicts manually or re-running a bunch of CI?
Instead of a new linter, I wrote a few lines of Python to call the GitHub API, and list me all the PRs that introduce new |
|
ACK 1a6323b -- code review only Rescanning open PRs after this is merged should be enough for CI to catch everything I think? Maybe close/reopen any PRs that still hit so CI gets rerun and errors on them? |
ebc4ab7 refactor: post Optional<> removal cleanups (fanquake) 57e980d scripted-diff: remove Optional & nullopt (fanquake) Pull request description: Same rationale & motivation as #21404, which turned out to be quite low in the number of potential conflicts. Lets see what the bot has to say here. ACKs for top commit: practicalswift: cr ACK ebc4ab7: patch looks correct jnewbery: utACK ebc4ab7 laanwj: Code review ACK ebc4ab7 Tree-SHA512: 550fbeef09b9d35ddefaa805d1755c18c8fd499c4b0f77ebfece8c20296a7abd1cf6c699e2261f92fe3552deeb7555ec2a2287ffe3ab9e98bb9f8612a4d43be3
Since requiring C++17, this is just pointless abstraction. I think we should just "tear the band-aid off" and remove it. Similar to the changes happening in #21366.
Also, having a comment saying this is deprecated doesn't prevent it's usage in new code. i.e : #20946 (comment).
The repository is fairly quiet at the moment, so any potential complaints about having to rebase should be minimal. Might as well get this over and done with.