Use MakeUnique to construct objects owned by unique

practicalswift commented at 9:45 pm on September 12, 2018: contributor

Use MakeUnique to construct objects owned by unique_ptrs.

Rationale:

MakeUnique ensures exception safety in complex expressions.
MakeUnique gives a more concise statement of the construction.

Use MakeUnique to construct objects owned by unique_ptrs

Rationale:
* MakeUnique ensures exception safety in complex expressions.
* MakeUnique gives a more concise statement of the construction.

81f81ec022

fanquake added the label Refactoring on Sep 12, 2018

DrahtBot commented at 0:19 am on September 13, 2018: member

#14224 (Add -fsanitize=integer Travis job. Document intentional and unintentional unsigned integer overflows (wraparounds) using annotations. by practicalswift)
#14010 (tests: Setup chain parameters (globalChainParams) when performing fuzzing initialization by practicalswift)
#13311 (Don’t edit Chainparams after initialization by jtimon)
#9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)

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.

practicalswift force-pushed on Sep 13, 2018

promag commented at 7:49 pm on September 13, 2018: member

Concept ACK. Could mention std::make_unique is only available in c++14 so MakeUnique is a temporary replacement.

practicalswift force-pushed on Sep 13, 2018

Add developer note about MakeUnique() preference da15d9d3d9

practicalswift force-pushed on Sep 13, 2018

practicalswift commented at 8:42 pm on September 13, 2018: contributor

@promag Good point! Added. Please re-review :-)

donaloconnor commented at 10:30 pm on September 13, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/commit/81f81ec022bce22c916b2964f5e38c318a828d9a :-)

ken2812221 commented at 1:32 am on September 14, 2018: contributor

utACK da15d9d3d9451b5cb3af137305a702174a9202b3

practicalswift commented at 11:30 am on September 18, 2018: contributor

@donaloconnor Thanks for the review! I’m afraid you didn’t review the latest commit. Would you mind reviewing da15d9d3d9451b5cb3af137305a702174a9202b3? :-)

promag commented at 11:34 am on September 18, 2018: member

utACK da15d9d.

donaloconnor commented at 12:30 pm on September 18, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/commit/da15d9d3d9451b5cb3af137305a702174a9202b3

MarcoFalke commented at 8:11 pm on September 20, 2018: member

Thanks, but this can be changed the next time someone touches the function(s). No need for a separate patch.

Pull requests without a rationale and clear improvement may be closed immediately.

Please provide clear motivation for your patch and explain how it improves Bitcoin Core user experience or Bitcoin Core developer experience significantly.

Any test improvements or new tests that improve coverage are always welcome.
Features are always welcome, but might be rejected early in the review process.
Bug fixes are most welcome when they come with steps to reproduce or an explanation of the potential issue as well as reasoning for the way the bug was fixed.
Refactoring changes are only accepted if they are required for a feature or bug fix or otherwise improve developer experience significantly. For example, most “code style” refactoring changes require a thorough explanation why they are useful, what downsides they have and why they significantly improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the developer notes, stylistic code changes are usually rejected.
All changes should have accompanying unit tests (see src/test/) or functional tests (see test/). PR authors should note which tests cover modified code. If no tests exist for a region of modified code, new tests should accompany the change.

Bitcoin Core has a thorough review process and even the most trivial change needs to pass a lot of eyes and requires non-zero or even substantial time effort to review. There is a huge lack of active reviewers on the project, so patches often sit for a long time.

MarcoFalke closed this on Sep 20, 2018

MarcoFalke commented at 8:35 pm on September 20, 2018: member

Feel free to submit the test and documentation changes separately, but we don’t have the resources right now to review refactoring of core code. Also I think we are not affected by “complex expressions” that require MakeUnique to ensures exception safety.

ryanofsky commented at 9:07 pm on September 20, 2018: member

we don’t have the resources right now to review refactoring of core code

Could we have a tag to distinguish PRs that were closed to due lack of resources from PRs that were closed due to more substantive issues (like brokenness or lack of agreement)? It might be useful to track where we are falling behind, and maybe nice to take edge off rejection for people who submit such PRs, sort of https://en.wikipedia.org/wiki/It%27s_not_you,_it%27s_me

MarcoFalke commented at 9:19 pm on September 20, 2018: member

@ryanofsky A closed pull request due to lack of author resources is tagged with “up for grabs”. A closed pull request due to lack of reviewer resources is usually left open until it is reviewed or closed due to a merge conflict and then also tagged with “up for grabs”.

For refactoring pull requests, I don’t see the need to place them “up for grabs”, since the same topics come up frequently every couple of months.

ryanofsky commented at 9:25 pm on September 20, 2018: member

Yes, I’m not suggesting as tagging them as “up for grabs”, I’m suggesting a new tag like “insufficient resources” to distinguish low priority PRs from PRs with actual substantive problems.

MarcoFalke referenced this in commit 920c090f63 on Sep 21, 2018

practicalswift deleted the branch on Apr 10, 2021

Munkybooty referenced this in commit 25c9e682f6 on Jun 24, 2021

Munkybooty referenced this in commit 5f87520aa3 on Jun 24, 2021

Munkybooty referenced this in commit 6dcbddbf10 on Jun 28, 2021

Munkybooty referenced this in commit 36ba5267df on Jun 29, 2021

Munkybooty referenced this in commit afe42bd422 on Jun 29, 2021

Munkybooty referenced this in commit e48e6b5864 on Jun 29, 2021

gades referenced this in commit 2bfd797aac on May 1, 2022

DrahtBot locked this on Aug 18, 2022

Use MakeUnique to construct objects owned by unique_ptrs #14211