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: align PriorityQueue and ArrayList API-wise #19960

Merged
merged 1 commit into from
May 20, 2024

Conversation

matklad
Copy link
Contributor

@matklad matklad commented May 13, 2024

ArrayList uses items slice to store len initialized items, while PriorityQueue stores capacity potentially uninitialized items.

This is a surprising difference in the API that leads to bugs!

tigerbeetle/tigerbeetle#1948

@matklad
Copy link
Contributor Author

matklad commented May 13, 2024

Note: I haven't run the tests locally. I'd love to, but there's a conspicuous lack of a single command that I can just copy-paste to get the tests going at https://github.com/ziglang/zig/wiki/Contributing#testing, and I am not motivated enough at this point to extract that command from the overall build instructions :)

@mochalins
Copy link

Note: I haven't run the tests locally. I'd love to, but there's a conspicuous lack of a single command that I can just copy-paste to get the tests going at https://github.com/ziglang/zig/wiki/Contributing#testing, and I am not motivated enough at this point to extract that command from the overall build instructions :)

I'm... not sure I understand how there isn't a single easily copyable command to run the tests. It's at the top of the section you linked here. That aside, the first subsection has an even more easy-to-run local test that you can copy paste...

https://github.com/ziglang/zig/wiki/Contributing#directly-testing-the-standard-library-with-zig-test

@matklad
Copy link
Contributor Author

matklad commented May 13, 2024

I have neither ./stage4/bin/zig nor zig in my path. I understand that I can spend time reading more docs about how to get those, but I am not motivated enough to do that, as, at this point, it's easier to just wait for CI

ArrayList uses `items` slice  to store len initialized items, while
PriorityQueue stores `capacity` potentially uninitialized items.

This is a surprising difference in the API that leads to bugs!

tigerbeetle/tigerbeetle#1948
@matklad matklad changed the title std: align PriorityQueue and ArrayList std: align PriorityQueue and ArrayList API-wise May 13, 2024
@rohlem
Copy link
Contributor

rohlem commented May 13, 2024

@matklad just FYI, I don't think having zig in your PATH is required:
zig abc xyz ... with zig in your PATH should yield the same result as specifying the full path as .../path-to-zig/zig abc xyz ... .
(Just be sure with std testing to test the lib/std of the zig executable you're using, lest you accidentally mix two different versions,
since import("std") will usually pick the one next to zig executable.)

@matklad
Copy link
Contributor Author

matklad commented May 13, 2024

The problem is not PATH per se, but rather the fact that I don’t know which Zig I need (or, rather, which command I need to type to get the right zig without understanding which zig is right one).

To clarify, I know that I personally, if I spend time reading the docs, will be able to get the command to build/download the right version of Zig. I went through this exercise a couple of times already during the past year. It’s just that I remember that being non-trivial (the docs specify like five different ways to do this, and it’s unclear which one I need), and that I’ve already forgot what I’ve learned back then :-)

@andrewrk
Copy link
Member

Thanks! Great patch.

@andrewrk andrewrk merged commit 9f4f43c into ziglang:master May 20, 2024
10 checks passed
@andrewrk andrewrk added standard library This issue involves writing Zig code for the standard library. release notes This PR should be mentioned in the release notes. breaking Implementing this issue could cause existing code to no longer compile or have different behavior. labels May 20, 2024
@matklad matklad deleted the matklad/priorities branch May 20, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior. release notes This PR should be mentioned in the release notes. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants