Skip to content

net: make CNode::m_inbound_onion public, initialize explicitly #21167

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.

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

Merged

Conversation

jonatack
Copy link
Member

@jonatack jonatack commented Feb 12, 2021

Refactoring only, no change in behavior. This is a quick follow-up to #20210 to address these review comments:

Changes:

  • make the CNode::m_inbound_onion class member public, update the Doxygen comment, drop the getter, and update the tests
  • remove the CNode::m_inbound_onion default value initialization in the ctor declaration and the member initializer in favor of always passing it explicitly to the ctor where we initialize it dynamically, to both clarify the caller code and to allow the compiler to warn if it is uninitialized in the ctor or omitted in the caller

nKeyedNetGroup(nKeyedNetGroupIn),
id(idIn),
nLocalHostNonce(nLocalHostNonceIn),
m_conn_type(conn_type_in),
nLocalServices(nLocalServicesIn),
m_inbound_onion(inbound_onion)
nLocalServices(nLocalServicesIn)
Copy link
Member Author

Choose a reason for hiding this comment

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

Argument order changed for -Wreorder initialization order (in this case public members first, then private)

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 13, 2021

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

Conflicts

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.

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK 2ee4a7a

@maflcko
Copy link
Member

maflcko commented Feb 15, 2021

review ACK 2ee4a7a 🏀

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK 2ee4a7a9ec68c75094685c06ec793b614f44c4ce 🏀
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUikCQwAmcVC6fMv+9n4gQS8nzxAp37R5inc1fOah+e7LO71NXIv2KYFtyd4JKk0
q5ZMK/mR2VQmSbyt18nsSZYK+FDobjqT2GWR5Z69/d+yOiZsWpgfVrhmyHom4CTB
AW4gk2QKPgYbsQZOUw5W/n8kNLVjYSSsbS7iX8Gav0mPryomJT+ymboi0JU72z3o
ol+B7XLf99muaPtVZVyubKXRZDsMHLmofqiRPbLGiUByF2Algl+67BMvKRCWct3Y
o6/oVC1zeRov9/0wC2zrtCFuhSy7q+EXR0jm99kW5L63YVb/czgUZiEuh8oPzPK5
VbxxpDlGzt82XoPzdWQPZKN1/uYsbt8oEHjr7N1SrhhF4qhMVlVIS6QN/nN52qnD
PnVxrBZnXNPZbVIu5Y6peh6Zkq4y/23CwSFag3nbzf+3RkDfbx7CP1RscDmSYRJH
Vl1SEBrWFHFwoLb0zoURPX0OGy8HQneO1hB3B+z0DKEpuvLytwE+jz/ax/NNc6gZ
2/dY9Uz7
=FVLs
-----END PGP SIGNATURE-----

Timestamp of file with hash e099c9167b3f072f4f1ab5a70468aaf73041ab7a8fb7856044b91115cae08bca -

@maflcko maflcko merged commit cb073be into bitcoin:master Feb 15, 2021
@jonatack jonatack deleted the m_inbound_onion-make-public-and-explicit branch February 15, 2021 14:37
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 15, 2021
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 4, 2022
Summary:
> net: make CNode::m_inbound_onion public, drop getter, update tests

> net: remove CNode::m_inbound_onion defaults for explicitness
>
> and to allow the compiler to warn if uninitialized in the ctor
> or omitted in the caller.

This is a backport of [[bitcoin/bitcoin#21167 | core#21167]]

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10973
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants