-
Notifications
You must be signed in to change notification settings - Fork 37.8k
docs: Improve netaddress comments #15718
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
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
dba9b63
to
b11dbf7
Compare
This is now ready for review. |
Concept ACK -- nice clarification! Will review. A minor nit: make sure "IPv4", "IPv6" or "IP" are capitalised consistently. Some cases of "ip" in the current version. |
b11dbf7
to
7d3ee5c
Compare
Fixed some wording and changed some cases of "ip" to "address." |
7d3ee5c
to
cf19e24
Compare
Addressed nits from @jonatack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK cf19e24. Nice change. Left some suggestions but feel free to ignore them.
cf19e24
to
fcf39fb
Compare
@ryanofsky Thanks for the detailed comments. Definitely clearer and better wordings. Fixed! |
Please squash and this should be ready for merge :-) |
fcf39fb
to
c99497c
Compare
Squashed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK c99497c. Just rebase, squash and a few suggested changes since last review. Since the PR is documentation only and the comments are easy to verify, this might be a good candidate for merge soon.
c99497c
to
303372c
Compare
- Improve IsRFC methods docs - Improve {Is,Set}Internal docs - Add tor methods docs - Add IsIPv{4,6} docs - Add IsValid docs - Add IsRoutable docs - Improve GetGroup docs - Add CService::GetSockAddr docs - Add CService::GetKey docs - Add CSubNet::Match docs - Add NetmaskBits docs - Add CNetAddr default constructor docs
Added doc for |
303372c docs: Improve netaddress comments (Carl Dong) Pull request description: Improves comments for `netaddress`, making them available to Doxygen. I think this is worthwhile because a lot of the code require some context (e.g., A lot of the things that we do to fit hostnames and tor addresses into `CNetAddr` is non-obvious, and documenting it is beneficial). ACKs for commit 303372: Tree-SHA512: 2a35784a01ed8ec5fdbe111a540192d31bde16afa96e4be97b0385daf290fc7469a66d7cb8905a70b920fad6a0e7400ca4e5da082d6e4af1d1aaccc0e8297720
8be3f30 netaddress: Update CNetAddr for ORCHIDv2 (Carl Dong) Pull request description: ``` The original ORCHID prefix was deprecated as of 2014-03, the new ORCHIDv2 prefix was allocated by RFC7343 as of 2014-07. We did not consider the original ORCHID prefix routable, and I don't see any reason to consider the new one to be either. ``` Would like to know if people think this kind of thing is even worth keeping the codebase updated for. Perhaps it'd be nice to write a devtool to pull the csv from [here](https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml) and generate the code. ACKs for commit 8be3f3: laanwj: utACK 8be3f30 ryanofsky: utACK 8be3f30. Only change since last review is rebasing after #15718 merge. Tree-SHA512: 7c93317f597b1a6c1443e12dd690010392edb9d72a479a8201970db7d3444fbb99a80b98026caad6fbfbebb455ab4035d2dde79bc9263bfd1d0398cd218392e1
8be3f30 netaddress: Update CNetAddr for ORCHIDv2 (Carl Dong) Pull request description: ``` The original ORCHID prefix was deprecated as of 2014-03, the new ORCHIDv2 prefix was allocated by RFC7343 as of 2014-07. We did not consider the original ORCHID prefix routable, and I don't see any reason to consider the new one to be either. ``` Would like to know if people think this kind of thing is even worth keeping the codebase updated for. Perhaps it'd be nice to write a devtool to pull the csv from [here](https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml) and generate the code. ACKs for commit 8be3f3: laanwj: utACK 8be3f30 ryanofsky: utACK 8be3f30. Only change since last review is rebasing after bitcoin#15718 merge. Tree-SHA512: 7c93317f597b1a6c1443e12dd690010392edb9d72a479a8201970db7d3444fbb99a80b98026caad6fbfbebb455ab4035d2dde79bc9263bfd1d0398cd218392e1
Summary: 303372c41a8d5c58a46cf9ed595e30e67bd0bc99 docs: Improve netaddress comments (Carl Dong) Pull request description: Improves comments for `netaddress`, making them available to Doxygen. I think this is worthwhile because a lot of the code require some context (e.g., A lot of the things that we do to fit hostnames and tor addresses into `CNetAddr` is non-obvious, and documenting it is beneficial). ACKs for commit 303372: Tree-SHA512: 2a35784a01ed8ec5fdbe111a540192d31bde16afa96e4be97b0385daf290fc7469a66d7cb8905a70b920fad6a0e7400ca4e5da082d6e4af1d1aaccc0e8297720 Backport of Core [[bitcoin/bitcoin#15718 | PR15718]] Test Plan: ninja Reviewers: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6494
8be3f30 netaddress: Update CNetAddr for ORCHIDv2 (Carl Dong) Pull request description: ``` The original ORCHID prefix was deprecated as of 2014-03, the new ORCHIDv2 prefix was allocated by RFC7343 as of 2014-07. We did not consider the original ORCHID prefix routable, and I don't see any reason to consider the new one to be either. ``` Would like to know if people think this kind of thing is even worth keeping the codebase updated for. Perhaps it'd be nice to write a devtool to pull the csv from [here](https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml) and generate the code. ACKs for commit 8be3f3: laanwj: utACK 8be3f30 ryanofsky: utACK 8be3f30. Only change since last review is rebasing after bitcoin#15718 merge. Tree-SHA512: 7c93317f597b1a6c1443e12dd690010392edb9d72a479a8201970db7d3444fbb99a80b98026caad6fbfbebb455ab4035d2dde79bc9263bfd1d0398cd218392e1
8be3f30 netaddress: Update CNetAddr for ORCHIDv2 (Carl Dong) Pull request description: ``` The original ORCHID prefix was deprecated as of 2014-03, the new ORCHIDv2 prefix was allocated by RFC7343 as of 2014-07. We did not consider the original ORCHID prefix routable, and I don't see any reason to consider the new one to be either. ``` Would like to know if people think this kind of thing is even worth keeping the codebase updated for. Perhaps it'd be nice to write a devtool to pull the csv from [here](https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml) and generate the code. ACKs for commit 8be3f3: laanwj: utACK 8be3f30 ryanofsky: utACK 8be3f30. Only change since last review is rebasing after bitcoin#15718 merge. Tree-SHA512: 7c93317f597b1a6c1443e12dd690010392edb9d72a479a8201970db7d3444fbb99a80b98026caad6fbfbebb455ab4035d2dde79bc9263bfd1d0398cd218392e1
8be3f30 netaddress: Update CNetAddr for ORCHIDv2 (Carl Dong) Pull request description: ``` The original ORCHID prefix was deprecated as of 2014-03, the new ORCHIDv2 prefix was allocated by RFC7343 as of 2014-07. We did not consider the original ORCHID prefix routable, and I don't see any reason to consider the new one to be either. ``` Would like to know if people think this kind of thing is even worth keeping the codebase updated for. Perhaps it'd be nice to write a devtool to pull the csv from [here](https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml) and generate the code. ACKs for commit 8be3f3: laanwj: utACK 8be3f30 ryanofsky: utACK 8be3f30. Only change since last review is rebasing after bitcoin#15718 merge. Tree-SHA512: 7c93317f597b1a6c1443e12dd690010392edb9d72a479a8201970db7d3444fbb99a80b98026caad6fbfbebb455ab4035d2dde79bc9263bfd1d0398cd218392e1
Improves comments for
netaddress
, making them available to Doxygen.I think this is worthwhile because a lot of the code require some context (e.g., A lot of the things that we do to fit hostnames and tor addresses into
CNetAddr
is non-obvious, and documenting it is beneficial).