refactor: Use C++11 default member initializers by maflcko · Pull Request #15109 · bitcoin/bitcoin

maflcko · 2019-01-05T11:02:07Z

Changes:

Remove unused constructors that leave some members uninitialized
Remove manual initialization in each constructor and prefer C++11 default member initializers

This is not a stylistic change, but a change that avoids bugs such as:

fix uninitialized read when stringifying an addrLocal fix uninitialized read when stringifying an addrLocal #14728
qt: Initialize members in WalletModel qt: Initialize members in WalletModel #12426
net: correctly initialize nMinPingUsecTime net: correctly initialize nMinPingUsecTime #6636
...

practicalswift · 2019-01-05T14:23:14Z

Concept ACK

From our developer notes:

Initialize all non-static class members where they are defined. If this is skipped for a good reason (i.e., optimization on the critical path), add an explicit comment about this

Rationale: Ensure determinism by avoiding accidental use of uninitialized values. Also, static analyzers balk about this. Initializing the members in the declaration makes it easy to spot uninitialized ones.
class A
{
    uint32_t m_count{0};
}

fanquake · 2019-01-05T14:58:23Z

Concept ACK

On macOS:

  CXX      libbitcoin_server_a-pow.o
  CXX      libbitcoin_server_a-rest.o
In file included from miner.cpp:17:
./net.h:414:33: error: expected ';' at end of declaration list
    bool setBannedIsDirty{false} GUARDED_BY(cs_setBanned);
                                ^
                                ;
1 error generated.

hebasto · 2019-01-05T15:26:46Z

Concept ACK.

DrahtBot · 2019-01-05T16:40:29Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14605 (Return of the Banman by dongcarl)

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.

donaloconnor · 2019-01-05T23:03:54Z

utACK fa2510d

Empact · 2019-01-06T00:57:05Z

src/wallet/rpcwallet.cpp

-        nAmount = 0;
-        nConf = std::numeric_limits<int>::max();
-        fIsWatchonly = false;
    }


nit: could drop this constructor (as it's equivalent to the default)

Note that this code is going away anyway as it's part of the account system

Empact · 2019-01-06T00:58:03Z

src/wallet/walletdb.cpp

-        fIsEncrypted = false;
-        fAnyUnordered = false;
-        nFileVersion = 0;
    }


nit: could drop this constructor

Empact · 2019-01-06T01:00:06Z

utACK fa2510d

practicalswift · 2019-01-06T09:30:31Z

utACK fa2510d modulo @Empact:s nits

promag · 2019-01-06T19:59:48Z

Big concept ACK.

laanwj · 2019-01-07T12:55:19Z

utACK fa2510d (checked that all the fields match)

fanquake · 2019-01-08T15:14:17Z

utACK fa2510d

fa2510d Use C++11 default member initializers (MarcoFalke) Pull request description: Changes: * Remove unused constructors that leave some members uninitialized * Remove manual initialization in each constructor and prefer C++11 default member initializers This is not a stylistic change, but a change that avoids bugs such as: * fix uninitialized read when stringifying an addrLocal #14728 * qt: Initialize members in WalletModel #12426 * net: correctly initialize nMinPingUsecTime #6636 * ... Tree-SHA512: 0f896f3b9fcc464d5fc7525f7c86343ef9ce9fb13425fbc68e9a9728fd8710c2b4e2fd039ee08279ea41ff20fd92b7185cf5cca95a0bcb6a5340a1e6f03cae6b

fac2f5e Use C++11 default member initializers (MarcoFalke) Pull request description: The second and last change on this topic (c.f. #15109). Split up because the diff would otherwise interleave, making review harder than necessary. This is not a stylistic change, but a change that avoids bugs such as: * fix uninitialized read when stringifying an addrLocal #14728 * qt: Initialize members in WalletModel #12426 * net: correctly initialize nMinPingUsecTime #6636 * ... Tree-SHA512: 547ae72b87aeaed5890eb5fdcff612bfc93354632b238d89e1e1c0487187f39609bcdc537ef21345e0aea8cfcf1ea48da432d672c5386dd87cf58742446a86b1

fa2510d Use C++11 default member initializers (MarcoFalke) Pull request description: Changes: * Remove unused constructors that leave some members uninitialized * Remove manual initialization in each constructor and prefer C++11 default member initializers This is not a stylistic change, but a change that avoids bugs such as: * fix uninitialized read when stringifying an addrLocal bitcoin#14728 * qt: Initialize members in WalletModel bitcoin#12426 * net: correctly initialize nMinPingUsecTime dashpay#6636 * ... Tree-SHA512: 0f896f3b9fcc464d5fc7525f7c86343ef9ce9fb13425fbc68e9a9728fd8710c2b4e2fd039ee08279ea41ff20fd92b7185cf5cca95a0bcb6a5340a1e6f03cae6b

maflcko added the Refactoring label Jan 5, 2019

maflcko force-pushed the Mf1901-ctorClean branch 3 times, most recently from fa1c03c to faebc6b Compare January 5, 2019 13:42

Use C++11 default member initializers

fa2510d

maflcko force-pushed the Mf1901-ctorClean branch from faebc6b to fa2510d Compare January 5, 2019 15:16

Empact reviewed Jan 6, 2019

View reviewed changes

src/wallet/walletdb.cpp

fIsEncrypted = false;

fAnyUnordered = false;

nFileVersion = 0;

}

Copy link

Contributor

Empact Jan 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could drop this constructor

laanwj merged commit fa2510d into bitcoin:master Jan 9, 2019

maflcko deleted the Mf1901-ctorClean branch January 9, 2019 14:07

maflcko mentioned this pull request Jan 10, 2019

[refactor] CNode: Use C++11 default member initializers #15144

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Conversation

maflcko commented Jan 5, 2019

Uh oh!

practicalswift commented Jan 5, 2019

Uh oh!

fanquake commented Jan 5, 2019

Uh oh!

hebasto commented Jan 5, 2019

Uh oh!

DrahtBot commented Jan 5, 2019

Conflicts

Uh oh!

donaloconnor commented Jan 5, 2019

Uh oh!

Empact Jan 6, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

Empact Jan 6, 2019

Choose a reason for hiding this comment

Uh oh!

Empact commented Jan 6, 2019

Uh oh!

practicalswift commented Jan 6, 2019

Uh oh!

promag commented Jan 6, 2019

Uh oh!

laanwj commented Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Jan 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

laanwj commented Jan 7, 2019 •

edited

Loading