Skip to content

refactor: Use C++11 default member initializers #15109

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jan 9, 2019
Merged

refactor: Use C++11 default member initializers #15109

merged 1 commit into from
Jan 9, 2019

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jan 5, 2019

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:

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

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
Copy link
Member

fanquake commented Jan 5, 2019

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
Copy link
Member

hebasto commented Jan 5, 2019

Concept ACK.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 5, 2019

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
Copy link
Contributor

utACK fa2510d

Empact
Empact reviewed Jan 6, 2019
View reviewed changes
src/wallet/rpcwallet.cpp
tallyitem()
{
nAmount = 0;
nConf = std::numeric_limits<int>::max();
fIsWatchonly = false;
}
Copy link
Contributor

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 (as it's equivalent to the default)

Copy link
Member

Choose a reason for hiding this comment

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

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

Empact
Empact reviewed Jan 6, 2019
View reviewed changes
src/wallet/walletdb.cpp
nKeys = nCKeys = nWatchKeys = nKeyMeta = m_unknown_records = 0;
fIsEncrypted = false;
fAnyUnordered = false;
nFileVersion = 0;
}
Copy link
Contributor

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

@Empact
Copy link
Contributor

Empact commented Jan 6, 2019

utACK fa2510d

@practicalswift
Copy link
Contributor

utACK fa2510d modulo @Empact:s nits

@promag
Copy link
Contributor

promag commented Jan 6, 2019

Big concept ACK.

@laanwj
Copy link
Member

laanwj commented Jan 7, 2019 •
edited
Loading

utACK fa2510d (checked that all the fields match)

@fanquake
Copy link
Member

fanquake commented Jan 8, 2019

utACK fa2510d

@laanwj laanwj merged commit fa2510d into bitcoin:master Jan 9, 2019
laanwj added a commit that referenced this pull request Jan 9, 2019
@laanwj
Merge #15109: refactor: Use C++11 default member initializers
3f12515 
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
@maflcko maflcko deleted the Mf1901-ctorClean branch January 9, 2019 14:07
@maflcko maflcko mentioned this pull request Jan 10, 2019
Merged
laanwj added a commit that referenced this pull request Jan 14, 2019
@laanwj
Merge #15144: [refactor] CNode: Use C++11 default member initializers
070eaf7 
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
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Aug 9, 2021
@laanwj @pravblockc
Merge bitcoin#15109: refactor: Use C++11 default member initializers
a7a5a1c 
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
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Aug 11, 2021
@laanwj @pravblockc
Merge bitcoin#15109: refactor: Use C++11 default member initializers
8e21d90 
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
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Aug 11, 2021
@laanwj @pravblockc
Merge bitcoin#15109: refactor: Use C++11 default member initializers
95d588e 
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
5tefan pushed a commit to 5tefan/dash that referenced this pull request Aug 12, 2021
@laanwj @5tefan
Merge bitcoin#15109: refactor: Use C++11 default member initializers
bfba8f2 
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laanwj laanwj laanwj left review comments

+1 more reviewer

@Empact Empact Empact left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@maflcko @practicalswift @fanquake @hebasto @DrahtBot @donaloconnor @Empact @promag @laanwj