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: Add std.ArrayList(T).transform #17661

Closed
wants to merge 1 commit into from
Closed

std: Add std.ArrayList(T).transform #17661

wants to merge 1 commit into from

Conversation

aloussase
Copy link

Transforming the data in an ArrayList is a common operation. I think it would be useful to have it as part of the standard library.

@aloussase
Copy link
Author

Also as I was writing the PR I noted something with std.testing.expectEqual. The first argument is called expected but the test for std.ArrayList(u32).getLastOrNull() seems to be providing the expected value as the second argument, and the actual value as the first one.

In the tests I made for this PR I put the expected value as the first argument, but this required casting to u32 because comptime_int was being inferred. I think to fix this situation one of two things may be done:

  1. Switch the order of arguments in std.testing.expectEqual
  2. Add a type parameter to std.testing.expectEqual to avoid having to cast the expected value

I don't know if this is a real problem, but it is a little confusing seeing the expected value being provided in the position of the actual value.

@squeek502
Copy link
Collaborator

squeek502 commented Oct 21, 2023

For the expectEqual stuff, see #4437 and #17431

@InKryption
Copy link
Contributor

I'm failing to see why this:

list.transform(u32, &output_list, struct {
    fn transform(item: u32) u32 {
        return item + 1;
    }
});

would be preferable to this:

for (list.items) |item| {
    output_list.appendAssumeCapacity(item + 1);
}

or even better, to this:

try output_list.resize(list.items.len);
for (output_list.items, list.items) |*out, item| {
    out.* = item + 1;
}

Given the historical rejection of #940 and similar proposals, I believe this should be closed.

@aloussase aloussase closed this Oct 21, 2023
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.

3 participants