Skip to content
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

[v2] misc: make uv_ip4_addr()/uv_ip6_addr() accept an unsigned short port instead of int #4130

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aloisklink
Copy link
Contributor

Change the port parameter of uv_ip4_addr() and uv_ip6_addr() from an int to an unsigned short.

Currently, the uv_ip4_addr() and uv_ip6_addr() functions cast the port to a uint16_t internally anyway, when calling htons().

Making the function accept unsigned short instead has two benefits:

  • Very slight performance increase, since we can avoid a unsigned short -> int -> unsigned short conversion.
  • Compilers can warn users of libuv if their port parameter to uv_ip4_addr() might be truncated.

ABI BREAKING CHANGE: This PR targets the master/v2 branch, as this change breaks the ABI. Although, it does not break the API (other than potentially causing some compiler warnings if the user calls this function with overflowing types), so I haven't bothered putting a message into the libuv v1 -> v2 migration guide.


The CI-docs CI will probably fail due to a broken link that's unrelated to this PR.

It's already been fixed in the v1.x branch thanks to PR #4128, but this PR targets the master branch, which doesn't yet have this fix.

Change the `port` parameter of `uv_ip4_addr()` and `uv_ip6_addr()` from
an `int` to an `unsigned short`.

Currently, the `uv_ip4_addr()` and `uv_ip6_addr()` functions cast the
`port` to a `uint16_t` internally anyway, when calling [`htons()`][1].

Making the function accept `unsigned short` instead has two benefits:
  - Very slight performance increase, since we can avoid a
    `unsigned short` -> `int` -> `unsigned short` conversion.
  - Compilers can warn users of `libuv` if their `port` parameter
    to `uv_ip4_addr()` might be truncated.

BREAKING CHANGE: This change breaks the ABI, but it does not break the
                 API (other than potentially causing some compiler
                 warnings if the user calls this function with
                 overflowing types).

[1]: https://linux.die.net/man/3/htons
@vtjnash
Copy link
Member

vtjnash commented Oct 30, 2023

I am somewhat inclined to suggest making it an unsigned int and merging into v1.x, since that is a minor API change, but not an ABI change. @bnoordhuis WDYT?

@aloisklink
Copy link
Contributor Author

aloisklink commented Nov 1, 2023

I am somewhat inclined to suggest making it an unsigned int and merging into v1.x, since that is a minor API change, but not an ABI change.

Going from int to unsigned int might be an ABI change on some really really really unusual system. But I believe this should be fine on every modern system, even on Unisys mainframes that use ones' complement for negative numbers, so 🤷

I'm not too fussed if this change is pushed back until v2 (even if v2 never comes out) if you don't want to take the risk of breaking the ABI on a weird system. It is pretty minor after all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants