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

slices: BinarySearch, BinarySearchFunc: document return value for multiple possible matches in slice #65446

Closed
markusheukelom opened this issue Feb 2, 2024 · 3 comments
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@markusheukelom
Copy link

markusheukelom commented Feb 2, 2024

Proposal Details

Currently the documentation of slices.BinarySearch says:

// BinarySearch searches for target in a sorted slice and returns the position
// where target is found, or the position where target would appear in the
// sort order; it also returns a bool saying whether the target is really found
// in the slice. The slice must be sorted in increasing order.
func BinarySearch[S ~[]E, E cmp.Ordered](x S, target E) (int, bool) {

To me, it is not clear what is returned if the target exists multiple times in x. It seems (looking at the implementation) it always returns the index of first appearance, which would be logical, and a good feature, but I cannot 100% infer this from the documentation.

Issued this as a issue as for now I have to assume that it can return any index of an element with target value, as I don't know if the current behaviour is incidental or guaranteed.

@gopherbot gopherbot added this to the Proposal milestone Feb 2, 2024
@seankhliao seankhliao changed the title proposal: slices: BinarySearch, BinarySearchFunc: make documentation clearer on what happens if the search slice contains multiple target values. slices: BinarySearch, BinarySearchFunc: document return value for multiple possible matches in slice Feb 2, 2024
@seankhliao seankhliao removed this from the Proposal milestone Feb 2, 2024
@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 2, 2024
@mknyszek mknyszek added this to the Backlog milestone Feb 2, 2024
@mknyszek
Copy link
Contributor

mknyszek commented Feb 2, 2024

@eliben
Copy link
Member

eliben commented Feb 5, 2024

sort.Find and sort.Search do promise the lowest index, so it may be reasonable to promise this in the documentation

@gopherbot
Copy link

Change https://go.dev/cl/562345 mentions this issue: slices: document that BinarySearch[Func] return earliest position

ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
Fixes golang#65446

Change-Id: I08dc512fb1f0101eb8aac8767cdf582360699559
Reviewed-on: https://go-review.googlesource.com/c/go/+/562345
Reviewed-by: Eli Bendersky <eliben@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants