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: internal dtoi and xtoi helpers have unusual API #16350

Closed
mdempsky opened this issue Jul 13, 2016 · 3 comments
Closed

net: internal dtoi and xtoi helpers have unusual API #16350

mdempsky opened this issue Jul 13, 2016 · 3 comments
Milestone

Comments

@mdempsky
Copy link
Member

mdempsky commented Jul 13, 2016

These internal helpers have signatures like:

func dtoi(s string, i0 int) ...
func xtoi(s string, i0 int) ...

and they parse an integer value from &s[i0]. Especially since the majority of callers use i0==0, it seems like it would be simpler / more idiomatic to just have the caller pass s[i0:] as an argument.

In particular, dtoi theoretically supports parsing negative integers, except it looks for the '-' character at s[0], not s[i0].

/cc @bradfitz @dpiddy

@mdempsky mdempsky added this to the Go1.8 milestone Jul 13, 2016
@bradfitz
Copy link
Contributor

Yes, those weird me out every time I encounter them.

@gopherbot
Copy link

CL https://golang.org/cl/27206 mentions this issue.

@gopherbot
Copy link

CL https://golang.org/cl/28414 mentions this issue.

gopherbot pushed a commit that referenced this issue Sep 7, 2016
https://golang.org/cl/27206 fixed the dtoi function such that
it now properly parses negative number. Ironically, this causes
several other functions that depended on dtoi to now (incorrectly)
parse negative numbers.

For example, ParseCIDR("-1.0.0.0/32") used to be rejected prior to the
above CL, but is now accepted even though it is an invalid CIDR notation.
This CL fixes that regression.

We fix this by removing the signed parsing logic entirely from dtoi.
It was introduced relatively recently in https://golang.org/cl/12447
to fix a bug where an invalid port was improperly being parsed as OK.
It seems to me that the fix in that CL to the port handling logic was
sufficient such that a change to dtoi was unnecessary.

Updates #16350

Change-Id: I414bb1aa27d0a226ebd4b05a09cb40d784691b43
Reviewed-on: https://go-review.googlesource.com/28414
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Mikio Hara <mikioh.mikioh@gmail.com>
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
@golang golang locked and limited conversation to collaborators Sep 2, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants