Skip to content
This repository has been archived by the owner on May 25, 2023. It is now read-only.

De-emphasize IPv4-mapped IPv6 handling, relegate it to stdlib compat. #108

Closed
wants to merge 1 commit into from

Conversation

danderson
Copy link
Member

Proposal implementation for #64, to de-emphasize auto-unmapping. Keep it for stdlib because stdlib makes ambiguous types, but otherwise constructors don't do unobvious things.

Copy link
Member

@mdlayher mdlayher left a comment

Choose a reason for hiding this comment

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

LGTM

// address.
func IPv6Raw(addr [16]byte) IP {
// IPFrom16 returns the IPv6 address given by the bytes in addr.
func IPFrom16(addr [16]byte) IP {
Copy link
Contributor

Choose a reason for hiding this comment

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

This violates the "new behavior, new name" adage. It's better to break existing callers than silently break them with a change in behavior.

If this will only return IPv6 now, maybe a better name is just IPv6 to match https://godoc.org/inet.af/netaddr#IPv4.

@@ -1289,6 +1280,10 @@ func (u uint128) or(m uint128) uint128 {
return uint128{u.hi | m.hi, u.lo | m.lo}
}

func (u uint128) not() uint128 {
Copy link
Contributor

Choose a reason for hiding this comment

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

the other three above have docs, even if internal

},
{
name: "4in6-unmap",
fn: FromStdIP,
std: net.ParseIP("1.2.3.4"),
std: net.ParseIP("1.2.3.4").To16(),
Copy link
Contributor

Choose a reason for hiding this comment

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

this change shouldn't be doing anything, right? I'd prefer you keep the old line alone and add a second test, even if it's currently redundant. It'd be interesting if it one of the cases failed in a future Go release.

@josharian josharian closed this Jan 9, 2021
@josharian josharian deleted the branch master January 9, 2021 01:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants