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

Question: Is it okay for Bump::grow to only receive a new_size, but not the entire new_layout? #143

Closed
yanchith opened this issue Feb 15, 2022 · 5 comments

Comments

@yanchith
Copy link
Contributor

Was doing some relaxing reading of bumpalo's source (commit b22416c) when I found the following:

Allocator::grow receives new_layout, but the Allocator trait impl in bumpalo only passes new_size to Bump::grow.

What happens, if Allocator::grow gets called with a new layout that has a greater alignment requirement? Docs of Allocator::grow say:

Returns Err if the new layout does not meet the allocator’s size and alignment constraints of the allocator, or if growing otherwise fails.

So I guess it would be ok to return AllocError if the alignment requirements got stricter with the new layout, but the trait impl currently ignores that.

I also could be missing something 😂

@fitzgen
Copy link
Owner

fitzgen commented Feb 16, 2022

@yanchith thanks for filing an issue! Yes, I think that Bump::grow should take a new layout so that it can handle the new alignment requirements (if any) as well.

Would you be up for making these changes and sending a pull request?

@yanchith
Copy link
Contributor Author

If you mean just the lazy version of returning AllocError, if the new alignment isn't automatically satisfied, then sure! I think this might also apply to Bump::shrink, so I'll look into that as well. I'll be afk for the rest of the week, but I think I could send the PR afterwards.

Btw are you privy to what lead to the decision of letting the caller request a different alignment when growing/shrinking in the allocator api?

@fitzgen
Copy link
Owner

fitzgen commented Feb 16, 2022

If you mean just the lazy version of returning AllocError, if the new alignment isn't automatically satisfied, then sure!

That's a good place to start, but it shouldn't be hard to add the more general support (just call alloc again).

Btw are you privy to what lead to the decision of letting the caller request a different alignment when growing/shrinking in the allocator api?

I am not. Might be worth bringing up with the wg-allocators folks, if you feel like going down the rabbit hole.

@yanchith
Copy link
Contributor Author

FYI I asked around and got pointed here regarding the design history: rust-lang/wg-allocators#5

@yanchith
Copy link
Contributor Author

yanchith commented Mar 1, 2022

I think we can now close this - feel free reopen if you want this for documentation.

@yanchith yanchith closed this as completed Mar 1, 2022
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

No branches or pull requests

2 participants