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

strconv: Atoi documentation is slightly misleading #29461

Closed
awulkan opened this issue Dec 29, 2018 · 9 comments
Closed

strconv: Atoi documentation is slightly misleading #29461

awulkan opened this issue Dec 29, 2018 · 9 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.

Comments

@awulkan
Copy link

awulkan commented Dec 29, 2018

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

$ go version 1.11.4

Does this issue reproduce with the latest release?

Yes.

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

Irrelevant.

go env Output
$ go env

What did you do?

Look at the documentation here: https://golang.org/src/strconv/atoi.go?s=5013:5045#L192

What did you expect to see?

An explanation of strconv.Atoi that matched what it actually did.

What did you see instead?

An explanation that is only half true.

Atoi returns the result of ParseInt(s, 10, 0) converted to type int.

It appears that Atoi doesn't just return the value of ParseInt(s, 10, 0), it only calls ParseInt as a last alternative, the "slow path". Without checking the code I was under the belief that I could just replace Atoi with ParseInt and it would basically run the exact same code.

It appears that the documentation was correct up until this commit:
46aa9f5#diff-cbef0f4cdbfb7c355433bb342a5ef463

@josharian
Copy link
Contributor

Genuine question: Why do you care?

@awulkan
Copy link
Author

awulkan commented Dec 30, 2018

@josharian What's the point of having documentation if it's outdated? I don't see why it would be a problem to update it to reflect the current state of the code, or make it more "high level" so it doesn't end up getting outdated like this the next time it's changed.

@ALTree
Copy link
Member

ALTree commented Dec 30, 2018

Please note that the point of the documentation is not to describe the way the function is implemented under the hood. The only purpose of the documentation associated with the function signature is to explain what the function does (in the sense: what will it return when you call it).

So, in this case, the following sentence

Atoi returns the result of ParseInt(s, 10, 0)

is not telling you that the Atoi code, literally, is return Parseint(...). It's just a concise way to explain what the function does. Basically, the doc it's telling you that Atoi is a shortcut for ParseInt calls on base 10 number strings.

IMO the documentation appears "outdated" to you because you're reading it too literally.

@ALTree
Copy link
Member

ALTree commented Dec 30, 2018

Anyway, here's a proposal for a slightly more high-level doc:

func Atoi(s string) (int, error)

Atoi interprets the string s as a base 10 integer, and returns its value as an int type. It is functionally equivalent to a call to ParseInt(s, 10, 0). See ParseInt doc for a description of the returned error's semantic.

@ALTree ALTree changed the title strconv.Atoi documentation is slightly misleading strconv: Atoi documentation is slightly misleading Dec 30, 2018
@awulkan
Copy link
Author

awulkan commented Dec 30, 2018

I'm sorry if I was a bit unclear. I never meant that I expected the comment to explain in great details what it did under the hood. I was just a bit surprised when the comment didn't match up with the reality. Afaik the "short path" is a lot faster than the ParseInt path, so there's a performance difference between Atoi and simply running ParseInt directly.

@bitfield
Copy link

Your honour, my client's statement never claimed that Atoi necessarily calls ParseInt, merely that it returns the same result as ParseInt. I move for a dismissal on the grounds that the defendant has no case to answer.

@awulkan
Copy link
Author

awulkan commented Dec 30, 2018

@bitfield I didn't come here to argue. If no one else thinks it's an issue then fine, simply don't change it and close this issue.

The documentation does claim that it returns the result of ParseInt, not the same result as ParseInt though. Which led me to believe that it simply called ParseInt (as it once did).

Right now the documentation does try to explain what happens under the hood, but the documentation got outdated when the commit I linked to was introduced.

@gopherbot
Copy link

Change https://golang.org/cl/155924 mentions this issue: strconv: make docs for Itoa and Atoi slightly higher level

@bitfield
Copy link

@bitfield I didn't come here to argue. If no one else thinks it's an issue then fine, simply don't change it and close this issue.

It's been fixed!

@ALTree ALTree added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 31, 2018
@golang golang locked and limited conversation to collaborators Dec 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants