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.

Sign up for GitHub

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

strings, bytes: divergent documentation for FieldsFunc functions #38630

Closed
kprav33n opened this issue Apr 23, 2020 · 3 comments
Closed

strings, bytes: divergent documentation for FieldsFunc functions #38630

kprav33n opened this issue Apr 23, 2020 · 3 comments
Labels
Documentation Issues describing a change to documentation. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.15

Comments

@kprav33n
Copy link
Contributor

kprav33n commented Apr 23, 2020 •
edited
Loading

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

$ go version
go version go1.14.2 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

Read the documentation for strings.FieldsFunc

What did you see?

I see that the documentation is out of date with respect to the following note.

FieldsFunc makes no guarantees about the order in which it calls f(c). If f does
not return consistent results for a given c, FieldsFunc may crash.

This was appropriate when FieldsForFunc was using a two-pass logic. However, when the function was optimized to address #19789, this note is not correct anymore. The new implementation does only one-pass over the input. Hence, we can safely remove this note.
kprav33n added a commit to kprav33n/go that referenced this issue Apr 23, 2020
@kprav33n
strings: Remove an obsolete doc note for FieldsFunc
83ac2d3 
Fixes golang#38630
kprav33n added a commit to kprav33n/go that referenced this issue Apr 23, 2020
@kprav33n
strings: remove an obsolete doc note for FieldsFunc
291b1b4 
Fixes golang#38630
@kprav33n kprav33n mentioned this issue Apr 23, 2020
Closed
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/229763 mentions this issue: strings: remove an obsolete doc note for FieldsFunc

@kprav33n kprav33n changed the title strings: Obsolete note in strings.FieldsFunc strings: obsolete note in strings.FieldsFunc Apr 23, 2020
@martisch
Copy link
Contributor

martisch commented Apr 25, 2020 •
edited
Loading

Some context why I left that requirement in when I changed to code to not do two passes:

This allows us to move to a two pass algorithm when lots of Fields are involved. This would be an optimization that would improve the current code by reducing allocations when the output has more than 32 Fields. I understand that since the current implementation will not crash and the optimization would still break code that didnt adhere to the comment so might invalidate the possibilty to change the code again even with the comment.

I would like to keep the part that the function used should return consistent results to allow for a two pass algorithm. Would that be ok before we ship go1.15 and cant go back anymore?

At least we should align the comment for bytes.FieldsFunc now that we changed strings.FieldsFunc.

go/src/bytes/bytes.go

Line 448 in a9f8f02

// FieldsFunc makes no guarantees about the order in which it calls f(c).

I will send a change to align the comments again and remove the part about crashing.

@martisch martisch reopened this Apr 25, 2020
@andybons andybons added Documentation Issues describing a change to documentation. NeedsFix The path to resolution is known, but the work has not been done. labels Apr 27, 2020
@andybons andybons added this to the Go1.15 milestone Apr 27, 2020
@andybons andybons changed the title strings: obsolete note in strings.FieldsFunc strings, bytes: divergent documentation for FieldsFunc functions Apr 27, 2020
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/230797 mentions this issue: bytes, strings: align requirements for functions passed to FieldFuncs

xujianhai666 pushed a commit to xujianhai666/go-1 that referenced this issue May 21, 2020
@kprav33n @xujianhai666
strings: remove an obsolete doc note for FieldsFunc
3797aff 
Fixes golang#38630

Change-Id: I0b2b693dd88821dcfc035cf552b687565bb55ef6
GitHub-Last-Rev: 291b1b4
GitHub-Pull-Request: golang#38631
Reviewed-on: https://go-review.googlesource.com/c/go/+/229763
Reviewed-by: Robert Griesemer <gri@golang.org>
xujianhai666 pushed a commit to xujianhai666/go-1 that referenced this issue May 21, 2020
@martisch @xujianhai666
bytes, strings: align requirements for functions passed to FieldFuncs
51e1a8b 
golang.org/cl/229763 removed the documentation of requirements of
the function passed to FieldsFunc. The current implementation does
not require functions to return consistent results but this had not
been the case for previous implementations.

Add the requirement for consistent results back to the documentation
to allow for future implementations to be more allocation efficient
for an output with more than 32 fields. This is possible with a two
pass algorithm first determining the number of fields used to allocate
the output slice and then splitting the input into fields.

While at it align the documentation of bytes.FieldsFunc with
strings.FieldFunc.

Fixes golang#38630

Change-Id: Iabbf9ca3dff0daa41f4ec930a21a3dd98e19f122
Reviewed-on: https://go-review.googlesource.com/c/go/+/230797
Run-TryBot: Martin Möhrmann <moehrmann@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@golang golang locked and limited conversation to collaborators Apr 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation Issues describing a change to documentation. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.15
Development

Successfully merging a pull request may close this issue.

strings: remove an obsolete doc note for FieldsFunc kprav33n/go
4 participants
@martisch @andybons @kprav33n @gopherbot