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

net: typo in IPNet.String documentation #33433

Closed
gaissmai opened this issue Aug 2, 2019 · 13 comments
Closed

net: typo in IPNet.String documentation #33433

gaissmai opened this issue Aug 2, 2019 · 13 comments
Labels

Comments

@gaissmai
Copy link

@gaissmai gaissmai commented Aug 2, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12.7 linux/amd64

Does this issue reproduce with the latest release?

currently latest stable

What operating system and processor architecture are you using (go env)?

What did you do?

read the documentation

 go doc -src net.IPNet.String
// String returns the CIDR notation of n like "192.0.2.1/24"
// ...

What did you expect to see?

// String returns the CIDR notation of n like "192.0.2.0/24"

What did you see instead?

// String returns the CIDR notation of n like "192.0.2.1/24"
@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Aug 2, 2019

It looks correct to me. 192.0.2.1 is the host IP, and /24 is the prefix. What is wrong here ?

@gaissmai

This comment has been minimized.

Copy link
Author

@gaissmai gaissmai commented Aug 2, 2019

Nope, the base IP address of 192.0.2.1/24 is 192.0.2.0, and the String() method use the base, see the test or the source:
https://play.golang.org/p/z05_nDz1EFP

@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Aug 2, 2019

Oh that's right.

@agnivade agnivade removed the WaitingForInfo label Aug 2, 2019
@agnivade agnivade changed the title net.IPNet.String(): typo in documentation net: typo in IPNet.String documentation Aug 2, 2019
@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Aug 3, 2019

@clairerhoda perhaps you might be interested in this issue?

@bharaththiruveedula

This comment has been minimized.

Copy link

@bharaththiruveedula bharaththiruveedula commented Aug 3, 2019

@odeke-em @agnivade if no one is working on it ... can I push the PR?

@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Aug 3, 2019

Up to you and @clairerhoda to figure out. I would suggest to wait for @clairerhoda to have a chance to respond.

@bharaththiruveedula

This comment has been minimized.

Copy link

@bharaththiruveedula bharaththiruveedula commented Aug 3, 2019

Sure. Will wait for @clairerhoda to decide.

@clairerhoda

This comment has been minimized.

Copy link

@clairerhoda clairerhoda commented Aug 5, 2019

@odeke-em @agnivade if @bharaththiruveedula has it done he can open the PR.

@bharaththiruveedula

This comment has been minimized.

Copy link

@bharaththiruveedula bharaththiruveedula commented Aug 6, 2019

sure... then I will send PR

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 6, 2019

Change https://golang.org/cl/189117 mentions this issue: net: fix the docs in IPNet.String

@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Aug 6, 2019

Sorry, actually this example is correct. I did not analyze @gaissmai's example in detail.

@gaissmai - what your playground example is doing with _, c, _ := net.ParseCIDR("192.0.2.1/24") is that now c is is actually 192.0.2.0/24 and not 192.0.2.1. You can see the godoc for https://golang.org/pkg/net/#ParseCIDR for details.

Please see the following code (https://play.golang.org/p/-1UYrVvjtTs):

package main

import (
	"fmt"
	"net"
)

func main() {
	ip := net.IPNet{IP: net.IPv4(192, 168, 1, 4), Mask: net.CIDRMask(24, 32)}
	fmt.Println(ip.String())
}

And as a further confirmation, (*IPNet).String calls networkNumberAndMask which has

if ip = n.IP.To4(); ip == nil {
	ip = n.IP
	if len(ip) != IPv6len {
		return nil, nil
	}
}

Therefore, I think the docs are correct and there is no issue here.

@gaissmai

This comment has been minimized.

Copy link
Author

@gaissmai gaissmai commented Aug 6, 2019

@agnivade Hi, sorry but I disagree. Your example is a little bit contrived. The type IPNet consists of the netnumber and related CIDR mask. Your example misuse the public fileds of type IPNet to put an IP address in this slot which is not a netnumber for the related CIDR mask..

type IPNet struct {
    IP   IP     // network number
    Mask IPMask // network mask
}

If you use ParseCIDR to generate the IPNet struct you will never get the wrong IP address in this field.

@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Aug 6, 2019

Well yes, but the docs are technically right. Although, 192.0.2.0/24 may be more appropriate for a real world example. Leaving to @mikioh @ianlancetaylor

EDIT: to clarify, I am agreeing with you. Just waiting for a second opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.