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

c-api: wasmtime_store_limiter #5761

Merged
merged 1 commit into from
Feb 13, 2023
Merged

c-api: wasmtime_store_limiter #5761

merged 1 commit into from
Feb 13, 2023

Conversation

LukasForst
Copy link
Contributor

This was not yet discussed, but as of now, there's no way how to use StoreLimits from the C API. This PR adds wasmtime_store_limiter that allows setting supported limits for the Store.

Not sure who to assign for this PR, judging from the past PRs for c-api @alexcrichton maybe?

@github-actions github-actions bot added the wasmtime:c-api Issues pertaining to the C API. label Feb 10, 2023
@github-actions
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

* for a Store. Instantiation will fail with an error if this limit is exceeded.
* This value defaults to 10,000.
*
* Use `0` for the parameters that should be kept on default values.
Copy link
Member

Choose a reason for hiding this comment

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

I think there's a case to be made for allowing precisely 0 of the above items, so could this instead perhaps use a negative value to indicate "use the default"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about this as well, but in that case we would need to use different types then usize and u32. What do you suggest using instead of these?

Copy link
Member

Choose a reason for hiding this comment

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

For some of these values the range of 0..i32::MAX is sufficient (or even isize::MAX). Otherwise though you could also add a dedicated struct with getters/setters where not calling a setter is equivalent to "none" or something like that. I don't know the best way to handle optional parameters in C myself.

Copy link
Contributor Author

@LukasForst LukasForst Feb 13, 2023

Choose a reason for hiding this comment

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

struct is not perfect, you'd end up with same problems, even if you use pointers on integers.. because then (afaik) you wouldn't be able to distinguish 0 and NULL.

Let's go with -1 as indication "to leave default" and i64 for all types. That way we keep the max ranges same and we will have a way to use 0 as a correct value.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Sounds reasonable to me!

@alexcrichton alexcrichton enabled auto-merge (squash) February 13, 2023 15:53
auto-merge was automatically disabled February 13, 2023 15:53

Pull request was closed

@alexcrichton alexcrichton reopened this Feb 13, 2023
@alexcrichton alexcrichton enabled auto-merge (squash) February 13, 2023 15:53
@alexcrichton alexcrichton merged commit 6cddc92 into bytecodealliance:main Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:c-api Issues pertaining to the C API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants