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

std.mem: add indexOfMin and indexOfMax #9915

Merged
merged 2 commits into from Jan 29, 2022
Merged

std.mem: add indexOfMin and indexOfMax #9915

merged 2 commits into from Jan 29, 2022

Conversation

zzyxyzz
Copy link
Contributor

@zzyxyzz zzyxyzz commented Oct 7, 2021

Find the index of the smallest/largest number in a slice, without traversing it twice.

closes #9891

lib/std/mem.zig Show resolved Hide resolved
lib/std/mem.zig Show resolved Hide resolved
var best = slice[0];
var index: usize = 0;
for (slice[1..]) |item, i| {
if (item < best) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comparison predicate should be specified by the user to allow for extra flexibility and custom non-scalar types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about that. The precedent from std.math.min and std.mem.min is to only support scalar types. A custom predicate would be good to have, but maybe open an issue first?

lib/std/mem.zig Outdated Show resolved Hide resolved
@zzyxyzz
Copy link
Contributor Author

zzyxyzz commented Oct 8, 2021

Thanks for the review, @LemonBoy.

I've added the assert and more tests to the new functions, as well as to min and max, whence they were copied.

@zzyxyzz
Copy link
Contributor Author

zzyxyzz commented Oct 8, 2021

Drone failure unrelated?

@N00byEdge
Copy link
Sponsor Contributor

Hold on, there needs to be a indexOfMinMax too that gets both in a single pass, right?

@zzyxyzz
Copy link
Contributor Author

zzyxyzz commented Oct 22, 2021

@N00byEdge

Hold on, there needs to be a indexOfMinMax too that gets both in a single pass, right?

In principle, yes. But I'm not sure what the function signature would be. There doesn't seem to be an std.mem.minMax() to take inspiration from, and I don't know what the Zig-standard way of returning multiple values is in a situation like that. Any suggestions?

@InKryption
Copy link
Contributor

InKryption commented Oct 22, 2021

How about something explicit like struct { min: usize, max: usize }? That's basically the current resolution reached so far in #498, as I understand it.
Edit: or maybe struct { index_min: usize, index_max: usize } would be more clear.

@zzyxyzz
Copy link
Contributor Author

zzyxyzz commented Oct 22, 2021

Added combined min-max functions in a separate commit.

@Snektron
Copy link
Collaborator

Snektron commented Oct 24, 2021

There is already an equivalent of the minIndex and maxIndex functions in std.sort:

zig/lib/std/sort.zig

Lines 1413 to 1418 in ee98d87

pub fn argMax(
comptime T: type,
items: []const T,
context: anytype,
comptime lessThan: fn (context: @TypeOf(context), lhs: T, rhs: T) bool,
) ?usize {

zig/lib/std/sort.zig

Lines 1361 to 1366 in ee98d87

pub fn argMin(
comptime T: type,
items: []const T,
context: anytype,
comptime lessThan: fn (@TypeOf(context), lhs: T, rhs: T) bool,
) ?usize {

@zzyxyzz
Copy link
Contributor Author

zzyxyzz commented Oct 24, 2021

Interesting. I wouldn't quite call it an equivalent, as these functions take an explicit comparator and corresponding context -- which I think is too verbose for general use. But if it's felt that argMin/argMax are good enough, then this PR can be closed.

@SpexGuy
Copy link
Contributor

SpexGuy commented Jan 10, 2022

Going to close and reopen to kick off CI

@SpexGuy SpexGuy closed this Jan 10, 2022
@SpexGuy SpexGuy reopened this Jan 10, 2022
@zzyxyzz
Copy link
Contributor Author

zzyxyzz commented Jan 11, 2022

Drone failure looks unrelated, but I'm not quite sure. Has it been seen before? Something about missing references to "ceill" and "florl".

For finding the minimum and maximum values (and indices)
in a slice in a single pass.
@Vexu Vexu merged commit 88edde4 into ziglang:master Jan 29, 2022
@zzyxyzz zzyxyzz deleted the indexOfMinMax branch January 29, 2022 14:00
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.

std.mem: add indexOfMin/Max
7 participants