Skip to content

refactor, net: Increase CNode data member encapsulation #20373

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 2 commits into from
Jan 11, 2021
Merged

refactor, net: Increase CNode data member encapsulation #20373

merged 2 commits into from
Jan 11, 2021

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Nov 11, 2020 •
edited
Loading

All protected CNode data members could be private.

@DrahtBot DrahtBot added the P2P label Nov 11, 2020
@hebasto
Copy link
Member Author

hebasto commented Nov 11, 2020

cc @troygiorshev @jnewbery

@jnewbery
Copy link
Contributor

Concept ACK making protected members private if they don't need to be protected. I'll review once #19673 is merged and this is rebased.

@practicalswift
Copy link
Contributor

Concept ACK

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 12, 2020 •
edited
Loading

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

Conflicts

No conflicts as of last run.

This was referenced Nov 12, 2020
Merged
Closed
Merged
Closed
promag
promag reviewed Nov 14, 2020
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Concept ACK.

I'd drop the "consolidate" commit. But if it's kept then it could be marked move-only. Also, there's an unnecessary public:.

This was referenced Dec 7, 2020
Merged
Merged
This was referenced Dec 20, 2020
Closed
Closed
@sipa
Copy link
Member

sipa commented Dec 27, 2020

utACK 75f7e47

@maflcko
Copy link
Member

maflcko commented Dec 27, 2020

Tend to NACK for the same reason: #19673 (review)

This was referenced Dec 28, 2020
Merged
Merged
Merged
@jnewbery
Copy link
Contributor

jnewbery commented Jan 6, 2021

@hebasto could you rebase now that #20816 is merged?

@hebasto
Copy link
Member Author

hebasto commented Jan 9, 2021

Updated 75f7e47 -> 7f20edf (pr20373.01 -> pr20373.02).

@jnewbery

@hebasto could you rebase now that #20816 is merged?

Done.

@promag

... if it's kept then it could be marked move-only.

Done.

Also, there's an unnecessary public:.

Fixed.

@DrahtBot DrahtBot mentioned this pull request Jan 9, 2021
Merged
@jnewbery
Copy link
Contributor

jnewbery commented Jan 9, 2021

ACK 7f20edf

Verified that this is move-only, except for making the protected members private.

Thanks for cleaning this up!

@hebasto
Copy link
Member Author

hebasto commented Jan 10, 2021

Updated 7f20edf -> 3642b2e (pr20373.02 -> pr20373.03) due to the conflict with #20881.

@jnewbery
Copy link
Contributor

utACK 3642b2e

@maflcko
Copy link
Member

maflcko commented Jan 11, 2021

review ACK 3642b2e 🏛

Show signature and timestamp

Signature:

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

review ACK 3642b2ed34e6609e8de558b352516daadb12cac1 🏛
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgpSgwAiwjdT13xFBxwJQmARjPmO8p8A7XcH2TDGrYMjehHgN9z/c6LajnsGLrf
HxU581vUH1HnWmjnSFdlQOKC/IDRiUDw9ogpakbjVi4Z1+KJnXEwJCDoCQepAKGA
adpBXg1jZLbLXlyhsiladQb5mAEZ0Cbru8NnXIriUP0SFgM7RGR7EaP9G0BUVqYx
3rpDxXtsUmtaZ7Y2/2SpTABh2UEK0ya1v1x8ILlWwz1YhqTEunSI8VwNffxCiVBn
V+oYGbKEyM/zg6x3MQnnX1plXgoz0XEOqmqqK12jtkTQAknWp7tQj7DhSCbSjLaS
ojE2LPFc5GhYDA0nvMq8N//Bm6yd6Z29WK5x8trAGmOEbpGxcyqrjN4Nnuw0O0Ai
hxt0XnQ6YMjeFvh5xzrIQEQwTd2bttrk6CiAf46hnwwmXwHbNwecTRQtj4+7LZlf
frOLEwBnFnQiOZjkiIXP0Q1L3pfQYKSNxr8b426QUBhQBNLojP+xBw4eLX3Gu3Iq
zSgvcVPd
=leU8
-----END PGP SIGNATURE-----

Timestamp of file with hash a0e7b38da83edfebd9fb41f063e4ec58f924d3f47816589b67b81a023d3dcd94 -

@maflcko maflcko merged commit 5cce607 into bitcoin:master Jan 11, 2021
@hebasto hebasto deleted the 201111-cnode branch January 11, 2021 12:40
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 11, 2021
Merge bitcoin#20373: refactor, net: Increase CNode data member encaps…
5d6c718 
…ulation

3642b2e refactor, net: Increase CNode data member encapsulation (Hennadii Stepanov)
acebb79 refactor, move-only: Relocate CNode private members (Hennadii Stepanov)

Pull request description:

  All protected `CNode` data members could be private.

ACKs for top commit:
  jnewbery:
    utACK 3642b2e
  MarcoFalke:
    review ACK 3642b2e 🏛

Tree-SHA512: 8435e3c43c3b7a3107d58cb809b8b5e1a1c0068677e249bdf0fc6ed24140ac4fc4efe2a280a1ee86df180d738c0c9e10772308690607954db6713000cf6e728d
@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.
Reviewers
1 more reviewer

@promag promag promag left review comments

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

Successfully merging this pull request may close these issues.
7 participants
@hebasto @jnewbery @practicalswift @DrahtBot @sipa @maflcko @promag