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

unicode/utf8: add "Rune"-less aliases #11743

Open
robpike opened this issue Jul 17, 2015 · 9 comments
Open

unicode/utf8: add "Rune"-less aliases #11743

robpike opened this issue Jul 17, 2015 · 9 comments
Milestone

Comments

@robpike
Copy link
Contributor

@robpike robpike commented Jul 17, 2015

The names in the utf8 package are clumsy. We could add nicer aliases, such as Decode for DecodeRune.

Decode(p []byte) (r rune, size int)
DecodeString(s string) (r rune, size int)
DecodeLast(p []byte) (r rune, size int)
DecodeLastString(s string) (r rune, size int)

similarly for Encode.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jul 17, 2015

I vote no. Unnecessary documentation cognitive load & godoc noise for minor improvement.

@robpike
Copy link
Contributor Author

@robpike robpike commented Jul 17, 2015

They are so not Go-like now and they always rub me the wrong way, but they are obviously not necessary.

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Jul 17, 2015
@adg
Copy link
Contributor

@adg adg commented Jul 17, 2015

The duplication would be more gross than the long names, IMO.
But I don't even use these packages.

@jimmyfrasche
Copy link
Member

@jimmyfrasche jimmyfrasche commented Jul 17, 2015

I've used the package a few times and been bothred by the unidiomatic names, but I'd be more bothered with having to jump back and forth trying to figure out what the difference was between Decode and DecodeRune only to discover there was none. It wouldn't be clear in the package's Index, at least.

Changing the documentation of DecodeRune to "deprecated: use Decode" only exchanges awkwardness for clutter.

Could this be marked Go2? That would be the best time to clean it up all at once and simple renamings are eminently go fix-able.

@jimmyfrasche
Copy link
Member

@jimmyfrasche jimmyfrasche commented Oct 28, 2016

With #17056 I don't have any objection since the deprecated versions would be hidden by default.

@robpike
Copy link
Contributor Author

@robpike robpike commented Oct 29, 2016

Well, at this point we could actually add Rune-less aliases (just kidding).

@mpvl
Copy link
Member

@mpvl mpvl commented Oct 29, 2016

@robpike: actually, there is something to say for using aliases here:

  • It makes the relation between old and new implementation explicit (possibly together with the deprecated comment), allowing for generic rewrite tools to update code using the deprecated versions automatically.
  • There is no loss of performance if the wrapper function cannot be inlined. These functions are called in tight loops.
@robpike
Copy link
Contributor Author

@robpike robpike commented Oct 29, 2016

@mpvl: At the moment, aliases are only across package boundaries, so using them in this case would require new, possibly internal packages. That is not worthwhile.

But they are there for refactoring, and if the restrictions are softened then of course that would work. That was my point.

@mpvl
Copy link
Member

@mpvl mpvl commented Oct 30, 2016

@r: Ah right, too bad. Yeah, the internal package approach is out of the question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.