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 (review).
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.
Seems fine, but let’s wait for the list of conflicts. Two minor style nits (feel free to ignore):
git rm is preferred over rm -f because it will fail if the file is not tracked by git594@@ -595,10 +595,9 @@ Common misconceptions are clarified in those sections:
595
596 - *Rationale*: This avoids memory and resource leaks, and ensures exception safety.
597
598-- Use `MakeUnique()` to construct objects owned by `unique_ptr`s.
599+- Use `std::make_unique` to construct objects owned by `unique_ptr`s.
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.
Reviewers, 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.
-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-
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.
There’s one with any ACKs, #20228, which has a single ACK on the latest commit hash.
There’s at least one, #21365, which introduces new MakeUnique usage, so I’ve commented there.
I’ve taken the git rm suggestion, and dropped std::make_unique from the developer docs.
utACK 1a6323bdbe20bdb7b1c907d8fa0333ffa88b21ff
Change looks correct. No comment on when this should be merged. I’ll defer to the wisdom of the maintainers :mage: :mage: :mage:
ACK https://github.com/bitcoin/bitcoin/commit/1a6323bdbe20bdb7b1c907d8fa0333ffa88b21ff 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 :eyes: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?
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.
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 MakeUnique<> usage. I’ve now left comments on all the relevant PRs advising that std::make_unique should be used. i.e: here, here and here.
ACK 1a6323bdbe20bdb7b1c907d8fa0333ffa88b21ff – 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?