-
Notifications
You must be signed in to change notification settings - Fork 650
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
Update memory allocation functions to use allocator user data #4043
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a note here, should we add WASM_MEM_ALLOC_WITH_USER_DATA
to a deprecation list and remove it on the next API break? Not sure if having this option disabled causes any performance issues, and I think we could improve the readability of the code by not having yet another configuration option.
FYI: the initial concept However, I agree with the idea of combining |
I agree with the merger, and it need to be informed that the custom allocator may need to consider compatibility issue. |
@loganek about " add WASM_MEM_ALLOC_WITH_USER_DATA to a deprecation list". I observed that there are no definitions for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@lum1n0us I didn't necessarily mean to add the deprecation attribute (although it'd be nice) but more to maintain a list of things to deprecate on the next breaking-API release (e.g. WAMR 3.x) so we can bundle all the breaking changes together and don't forget about this one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l
fix #4024