refactor: Drop CCoinControl::SetNull #21714

pull promag wants to merge 1 commits into bitcoin:master from promag:2021-04-coincontrol changing 3 files +13 −31

promag commented at 1:02 AM on April 17, 2021: member
The only external call to SetNull is changed as follow
```
- m_coin_control->SetNull();
+ m_coin_control = std::make_unique<CCoinControl>();
```
DrahtBot added the label GUI on Apr 17, 2021
DrahtBot added the label Refactoring on Apr 17, 2021
DrahtBot added the label Wallet on Apr 17, 2021
MarcoFalke removed the label GUI on Apr 17, 2021
practicalswift commented at 7:35 AM on April 17, 2021: contributor
Concept ACK: default member initialisation is simply better. Thanks for cleaning this up.
The only external call to SetNull is changed as follow

- m_coin_control->SetNull(); + m_coin_control.reset(new CCoinControl);
Could be made more idiomatic as m_coin_control = std::make_unique<CCoinControl>()?

Rationale:

in src/wallet/coincontrol.cpp:11 in 5b428548b5 outdated

   5 | @@ -6,21 +6,8 @@
   6 |  
   7 |  #include <util/system.h>
   8 |  
   9 | -void CCoinControl::SetNull()
  10 | +CCoinControl::CCoinControl()
  11 |  {
  12 |      destChange = CNoDestination();

practicalswift commented at 7:39 AM on April 17, 2021:

descChange could be default initialized like the other member variables?

in src/wallet/coincontrol.h:45 in 5b428548b5 outdated
```
  43 | @@ -44,20 +44,15 @@ class CCoinControl
  44 |      //! Avoid partial use of funds sent to a given address
  45 |      bool m_avoid_partial_spends;
```
practicalswift commented at 7:42 AM on April 17, 2021:

Could be default initialised to DEFAULT_AVOIDPARTIALSPENDS?

MarcoFalke commented at 8:02 AM on April 17, 2021:

or made const, which also avoids uninitialized reads

promag commented at 8:31 AM on April 17, 2021:

It's changed in wallet.cpp.

promag commented at 8:55 AM on April 17, 2021:

Initialised to DEFAULT_AVOIDPARTIALSPENDS.
practicalswift changes_requested
promag force-pushed on Apr 17, 2021
promag force-pushed on Apr 17, 2021
DrahtBot commented at 10:27 AM on April 17, 2021: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #21359 (rpc: include_unsafe option for fundrawtransaction by t-bast)
- #17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)
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.
theStack commented at 3:24 PM on April 18, 2021: member
Concept ACK

Some suggestions:
- I'd not change the assignment to m_coin_control twice, but rather set it to the suggested form m_coin_control = std::make_unique<CCoinControl>(); in the first commit already
- A short mention about the second refactoring commit (which seems to be independent) in the PR description would be helpful for reviewers
refactor: Drop CCoinControl::SetNull c5a470eee1
promag force-pushed on Apr 18, 2021
promag commented at 8:17 PM on April 18, 2021: member

@theStack thanks, fixed 1st commit and moved the 2nd to https://github.com/bitcoin-core/gui/pull/284.
theStack approved
theStack commented at 12:01 AM on April 20, 2021: member

Code-Review ACK c5a470eee1ac864b7c02b6a1669327b68411d806