Skip to content

Conversation

dongcarl
Copy link
Contributor

@dongcarl dongcarl commented Apr 1, 2019

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).

@dongcarl dongcarl added the Docs label Apr 1, 2019
@fanquake fanquake added the P2P label Apr 1, 2019
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 1, 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:

  • #15689 (netaddress: Update CNetAddr for ORCHIDv2 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.

@dongcarl dongcarl force-pushed the 2019-04-netaddr-comments branch from dba9b63 to b11dbf7 Compare April 2, 2019 15:43
@dongcarl dongcarl marked this pull request as ready for review April 2, 2019 15:44
@dongcarl dongcarl changed the title [WIP] docs: Improve netaddress comments docs: Improve netaddress comments Apr 2, 2019
@dongcarl
Copy link
Contributor Author

dongcarl commented Apr 2, 2019

This is now ready for review.

@practicalswift
Copy link
Contributor

practicalswift commented Apr 2, 2019

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.

@dongcarl dongcarl force-pushed the 2019-04-netaddr-comments branch from b11dbf7 to 7d3ee5c Compare April 2, 2019 17:56
@dongcarl
Copy link
Contributor Author

dongcarl commented Apr 2, 2019

Fixed some wording and changed some cases of "ip" to "address."

@dongcarl dongcarl force-pushed the 2019-04-netaddr-comments branch from 7d3ee5c to cf19e24 Compare April 4, 2019 17:12
@dongcarl
Copy link
Contributor Author

dongcarl commented Apr 4, 2019

Addressed nits from @jonatack

Copy link
Contributor

@ryanofsky ryanofsky left a 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.

@dongcarl dongcarl force-pushed the 2019-04-netaddr-comments branch from cf19e24 to fcf39fb Compare April 8, 2019 15:50
@dongcarl
Copy link
Contributor Author

dongcarl commented Apr 8, 2019

@ryanofsky Thanks for the detailed comments. Definitely clearer and better wordings. Fixed!

@practicalswift
Copy link
Contributor

Please squash and this should be ready for merge :-)

@dongcarl dongcarl force-pushed the 2019-04-netaddr-comments branch from fcf39fb to c99497c Compare April 8, 2019 20:59
@dongcarl
Copy link
Contributor Author

dongcarl commented Apr 8, 2019

Squashed.

Copy link
Contributor

@ryanofsky ryanofsky left a 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.

@dongcarl dongcarl force-pushed the 2019-04-netaddr-comments branch from c99497c to 303372c Compare April 10, 2019 15:47
- 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
@dongcarl
Copy link
Contributor Author

Added doc for CNetAddr's default constructor as its implicit properties are used elsewhere in the codebase.

@laanwj laanwj merged commit 303372c into bitcoin:master Apr 11, 2019
laanwj added a commit that referenced this pull request Apr 11, 2019
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
laanwj added a commit that referenced this pull request Jun 6, 2019
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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 6, 2019
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
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jun 9, 2020
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 16, 2021
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
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Jan 18, 2021
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 18, 2021
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
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants