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 pointer stability locks to array list #19676

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

Conversation

emar-kar
Copy link
Contributor

Closes #19326

lib/std/array_list.zig Outdated Show resolved Hide resolved
Co-authored-by: Andrew Kelley <[email protected]>
Copy link

@vortexofdoom vortexofdoom left a comment

Choose a reason for hiding this comment

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

clone shouldn't require pointers to be unlocked, right? The only allocations are for the newly cloned ArrayList, and the original allocation is unchanged.

lib/std/array_list.zig Outdated Show resolved Hide resolved
lib/std/array_list.zig Outdated Show resolved Hide resolved
@emar-kar
Copy link
Contributor Author

emar-kar commented Sep 4, 2024

@vortexofdoom Do we consider async here? if we do, then if I try to clone it while it's locked by the update operation, it can corrupt the result. Or am I missing something?

@emar-kar
Copy link
Contributor Author

emar-kar commented Sep 4, 2024

And I was thinking if appendSliceAssumeCapacity should lock the array as well?

@vortexofdoom
Copy link

Interesting point. I guess my gut instinct would be to avoid making things that should be trivially possible in non-async settings impossible for the sake of async. In an async setting you can manually lock it before a clone.

Appending, even assuming capacity, could "invalidate" a pointer to the last element or similar, so that seems maybe less objectionable, but I could see the argument for removing it.

Although I guess the argument exists that you won't be locking much in a non-async context, so it may be a non-issue.

@emar-kar
Copy link
Contributor Author

emar-kar commented Sep 4, 2024

I see the point for removing assertion from clone. And actually I do support it. It's easy to bring it back in case

Anyway, the question about appendSliceAssumeCapacity is still open for discussion.

@vortexofdoom
Copy link

Looking at it, I think appendSliceAssumeCapacity is also better without. There's already an assertion that the items can fit, it's supposed to be the utility function for actually doing the thing under the hood, I think if it requires being unlocked suddenly there's a case for making an even deeper function that doesn't require it being unlocked.

The buck has to stop somewhere, and I think it's fine to stop short of functions with assume in the name.

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.

introduce pointer stability safety locks to array lists
3 participants