Skip to content

Prefer initialization to assignment in constructors. Prefer in-class initializers to member initializers in constructors for constant initializers. #13766

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
Closed
practicalswift wants to merge 5 commits into from
Closed

Prefer initialization to assignment in constructors. Prefer in-class initializers to member initializers in constructors for constant initializers. #13766

practicalswift wants to merge 5 commits into from

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Jul 26, 2018 •
edited
Loading

Initialize members in initialization lists. Prefer in-class initializers to member initializers in constructors for constant initializers.

Rationale:

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 26, 2018 •
edited
Loading

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.

maflcko
maflcko reviewed Jul 26, 2018
View reviewed changes
src/primitives/transaction.cpp Outdated
Copy link
Member

@maflcko maflcko Jul 26, 2018

Choose a reason for hiding this comment

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

Could put a \n before the : to avoid having excessively long lines, no? (Feddback also applies to other places with long lines)

@practicalswift practicalswift force-pushed the initialize-members-in-initialization-list branch from b61c902 to 5e80e93 Compare July 26, 2018 16:20
@practicalswift
Copy link
Contributor Author

practicalswift commented Jul 26, 2018

@MarcoFalke Good point! Newlines added. Please re-review :-)

@promag
Copy link
Contributor

promag commented Jul 26, 2018

Concept ACK.

Empact
Empact reviewed Jul 26, 2018
View reviewed changes
src/wallet/wallet.cpp Outdated
Copy link
Contributor

@Empact Empact Jul 26, 2018

Choose a reason for hiding this comment

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

nit: this one too?

Empact
Empact reviewed Jul 26, 2018
View reviewed changes
src/script/standard.cpp Outdated
Copy link
Contributor

@Empact Empact Jul 26, 2018

Choose a reason for hiding this comment

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

nit: whitespace

Copy link
Contributor Author

@practicalswift practicalswift Jul 26, 2018

Choose a reason for hiding this comment

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

@Empact Where do you want the whitespace added/removed? :-)

Copy link
Member

@maflcko maflcko Jul 26, 2018

Choose a reason for hiding this comment

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

Probably leave everything in a single line?

Copy link
Contributor

@Empact Empact Jul 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

clang-format-diff.py accepts at least:

explicit CScriptVisitor(CScript* scriptin) : script(scriptin) {}
explicit CScriptVisitor(CScript* scriptin) : script(scriptin)
{
}

For an empty set, like Marco I like the former.

Copy link
Contributor

@Empact Empact Jul 26, 2018

Choose a reason for hiding this comment

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

Could also apply to CompareInvMempoolOrder, FreespaceChecker, etc.

@practicalswift practicalswift force-pushed the initialize-members-in-initialization-list branch 5 times, most recently from 915e75a to 871d81c Compare August 2, 2018 11:55
@practicalswift
Copy link
Contributor Author

practicalswift commented Aug 2, 2018

@Empact Thanks for reviewing. Feedback addressed. Please re-review :-)

@DrahtBot DrahtBot mentioned this pull request Aug 23, 2018
Closed
1 task
@laanwj
Copy link
Member

laanwj commented Aug 29, 2018

Some of these initializers (those with constant values) could move to the class declaration itself

@practicalswift practicalswift force-pushed the initialize-members-in-initialization-list branch from 2a2f614 to eefb1b1 Compare August 29, 2018 13:50
@DrahtBot DrahtBot mentioned this pull request Aug 29, 2018
Closed
@practicalswift practicalswift force-pushed the initialize-members-in-initialization-list branch 3 times, most recently from 27c71db to 261a470 Compare August 30, 2018 09:14
@practicalswift practicalswift changed the title Initialize members in initialization lists Prefer initialization to assignment in constructors. Prefer in-class initializers to member initializers in constructors for constant initializers. Aug 30, 2018
@practicalswift practicalswift force-pushed the initialize-members-in-initialization-list branch from 261a470 to 6ee270e Compare August 30, 2018 09:23
@practicalswift
Copy link
Contributor Author

practicalswift commented Aug 30, 2018

@laanwj Now preferring in-class initializers to member initializers in constructors for constant initializers. Please re-review the PR.

Please note that a redundant call to filterInventoryKnown.reset() has been removed. That looks correct, right?

@practicalswift practicalswift force-pushed the initialize-members-in-initialization-list branch from 6ee270e to 5ff5fa7 Compare August 30, 2018 09:52
@practicalswift
Copy link
Contributor Author

practicalswift commented Aug 30, 2018

Also added a developer note: "Prefer initialization to assignment in constructors"

@practicalswift practicalswift mentioned this pull request Aug 31, 2018
Closed
@DrahtBot DrahtBot mentioned this pull request Aug 31, 2018
Closed
@practicalswift practicalswift force-pushed the initialize-members-in-initialization-list branch from 5ff5fa7 to 157ab56 Compare September 5, 2018 09:38
This was referenced Sep 20, 2018
Merged
Merged
@practicalswift practicalswift deleted the initialize-members-in-initialization-list branch April 10, 2021 19:35
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@Empact Empact Empact left review comments

@maflcko maflcko maflcko 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.

7 participants

@practicalswift @DrahtBot @promag @laanwj @Empact @maflcko @fanquake