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

Jump to bottom

proposal: net/netip: add To6 to wrap an IPv4 address into an IPv6 one #51833

Closed
favonia opened this issue Mar 21, 2022 · 14 comments
Closed

proposal: net/netip: add To6 to wrap an IPv4 address into an IPv6 one #51833

favonia opened this issue Mar 21, 2022 · 14 comments
Labels
FrozenDueToAge Proposal
Projects
Proposals (old)
Milestone
Proposal

Comments

@favonia
Copy link
Contributor

favonia commented Mar 21, 2022 •
edited

Currently, there is no efficient way to wrap an IPv4 address into an IPv6 one. The most sensible way is to do the following:

ip6 := AddrFrom16(ip4.As16())

But this is slow and not allocation-free. But this seems to be unnecessarily involved. I wish there would be a new method To6 that implements the same functionality but without allocation: (Note: I originally proposed Map here, but that name did not gain wide support.)

ip6 := ip4.To6()

Here is a possible implementation of the proposed To6 method:

func (ip Addr) To6() Addr {
	if ip.z == z4 {
		ip.z = z6noz
	}
	return ip
}

As an invariant, for any valid IPv4 address ip, ip.To6().Unmap() is always equal to ip.

EDIT: Following @rsc's comment, I removed the unsubstantiated claim that As16 is allocating.
@gopherbot gopherbot added this to the Proposal milestone Mar 21, 2022
@favonia favonia mentioned this issue Mar 21, 2022
Merged
3 tasks
@ianlancetaylor
Copy link
Contributor

It doesn't seem to me that Map is the best name for this method, despite the existence of the Unmap method.

CC @bradfitz @josharian

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Mar 21, 2022
@favonia
Copy link
Contributor Author

favonia commented Mar 21, 2022

It doesn't seem to me that Map is the best name for this method, despite the existence of the Unmap method.

Yeah, it's a bit unusual. I don't really have an opinion on names. 🙂

@hopehook
Copy link
Member

How about:

  1. Mapping
  2. Mapped
  3. MapTo

@favonia favonia mentioned this issue Mar 23, 2022
Closed
@favonia
Copy link
Contributor Author

favonia commented May 1, 2022 •
edited

It doesn't seem to me that Map is the best name for this method

@ianlancetaylor How about To6? This seems to go well with Is4, Is6, and Is4In6, and will remain distinct from As16 that is dealing with (16) bytes.

@favonia favonia changed the title proposal: net/netip: add Map that wraps an IPv4 address into an IPv6 one proposal: net/netip: add To6 that wraps an IPv4 address into an IPv6 one May 1, 2022
@favonia
Copy link
Contributor Author

favonia commented May 10, 2022

I rewrote the proposal with a better method name To6 and greater clarity.

@favonia favonia changed the title proposal: net/netip: add To6 that wraps an IPv4 address into an IPv6 one proposal: net/netip: add To6 to wrap an IPv4 address into an IPv6 one May 10, 2022
@rsc
Copy link
Contributor

rsc commented Jun 1, 2022

AddrFrom16(ip4.As16()) shouldn't be allocating, nor particularly slow.
Do we need a separate method to make that a shorter name?

@rsc rsc moved this from Incoming to Active in Proposals (old) Jun 1, 2022
@rsc
Copy link
Contributor

rsc commented Jun 1, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@favonia
Copy link
Contributor Author

favonia commented Jun 1, 2022 •
edited

AddrFrom16(ip4.As16()) shouldn't be allocating, nor particularly slow.

@rsc Thank you. I tried to play with the compiler and could not prove the allocation (not being familiar with the Go Assembler). I also agree that AddrFrom16(ip4.As16()) is not particularly slow. However, I still think To6 is much more intuitive than AddrFrom16(ip4.As16()), and it complements the existing Unmap method nicely. In any case, I appreciate that the review group is considering it.

@rsc
Copy link
Contributor

rsc commented Jun 8, 2022

At the moment we have very little evidence that this comes up often.
For a rare operation it's fine to have to write AddrFrom16(ip4.As16()).
Do you have any evidence that this arises frequently?

@favonia
Copy link
Contributor Author

favonia commented Jun 8, 2022 •
edited

Do you have any evidence that this arises frequently?

No, but I don't think we can reach a conclusion now because netip is still new and the old library does not really distinguish them. In any case, I understand the conservation towards the standard library.

@rsc
Copy link
Contributor

rsc commented Jun 15, 2022

It sounds like we should likely decline this, but we can always revisit if we get new information about how often this comes up.

@rsc
Copy link
Contributor

rsc commented Jun 15, 2022

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Jun 15, 2022
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Jun 22, 2022
@rsc
Copy link
Contributor

rsc commented Jun 22, 2022

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc closed this as completed Jun 22, 2022
@favonia favonia mentioned this issue Oct 22, 2022
Closed
@favonia
Copy link
Contributor Author

favonia commented Nov 15, 2022

It sounds like we should likely decline this, but we can always revisit if we get new information about how often this comes up.

For people who are interested, #54365 is the new incarnation of this proposal, and there is new information about efficiency and usage there (which are the two main reasons for the declination of this PR). I think we could consider revisiting it.

@golang golang locked and limited conversation to collaborators Nov 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal
Projects
No open projects
Proposals (old)
Declined
Milestone
Proposal
Development

No branches or pull requests
5 participants
@rsc @favonia @ianlancetaylor @hopehook @gopherbot