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

Improve std.sort.binarySearch's return type, fix std.sort functions to adhere to their documentation #22300

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Fri3dNstuff
Copy link
Contributor

This PR includes a couple of changes in std.sort, for the binary search functions family:

std.sort.binarySearch's return type has been changed from ?usize to struct { usize, bool } - that is because there is still meaningful information to return to the caller, even if no acceptable item was found, namely: the index where an acceptable item could be inserted, while preserving the input slice's order (such index is unique).

The other changes made are to correct the implementations of the search functions that accept a trisection callback; those were out of sync with their documentation ever since #21373 was implemented:

Trisection callbacks are expected to classify the slice's values as either too large (.gt), too small (.lt), or within target (.eq). This formulation is independent of any concrete key that may or may not exist and be compared against, and so is the most reasonable formulation in the generic case. This has been exemplified in the documentation since #20927:

`items` must be sorted in ascending order with respect to `compareFn`:

[0]                                                   [len]
┌───┬───┬─/ /─┬───┬───┬───┬─/ /─┬───┬───┬───┬─/ /─┬───┐
│.lt│.lt│ \ \ │.lt│.eq│.eq│ \ \ │.eq│.gt│.gt│ \ \ │.gt│
└───┴───┴─/ /─┴───┴───┴───┴─/ /─┴───┴───┴───┴─/ /─┴───┘
├─────────────────┼─────────────────┼─────────────────┤
 ↳ zero or more    ↳ zero or more    ↳ zero or more

However, #20927's formulation turned out to be quite confusing for the most common, specific case, where in fact there is a concrete key to compare the items to, passed via the context argument that the trisection callbacks accept. Forwarding the arguments to std.math.order to get an ascending order will require them to be swapped, making it appear like the values' relation is backwards.

#21373 introduced changes to the functions' implementations, like adding .invert() calls to lowerBound and upperBound's generated predicates - these superficially fixed the issues, and indeed resolved the confusion around concrete-key use cases; however, the changes also broke the generic case, making it expect inverted inputs. The proper solution is to maintain the internal logic of the functions, and instead swap the T and context arguments in the trisection callbacks (This was not originally done in #20927 as the precedent for callbacks is to take context as the first argument. Retrospectively, the confusion this led to vastly outweighs the consistency benefit).

The changes in this PR mean that:

  • No changes will have to be done for most calls, those that treat context as a key (the changes to such calls in this PR only rename arguments).
  • Complex calls to the search functions (those that return std.math.Order values directly, instead of forwarding to std.math.order) will have to switch .lts and .gts around, again making their behaviour consistent with the functions' documentation and intent.

The PR also tweaks the doc comments somewhat, explaining the functions' main goals and preconditions in the head sentence. compareFn is renamed to trisectFn for cases where a comparison, implying an order, is not the most natural way to think of the sections (e.g. three sections for red, green, and blue values). The type annotations of trisectFn get a probed: argument name, to instil that the first argument is now the one taken from the slice, in case the types themselves weren't an obvious indication.

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

Successfully merging this pull request may close these issues.

1 participant