-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add annotation support for string parameters #507
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.
I like where this PR is going, at the same time I don't know how we can identify and generate the code for this "specialization".
A note on using Random in tests and LGTM, thanks!
@WasmExport | ||
public void randomGet(Memory memory, int ptr, int len) { | ||
byte[] data = new byte[len]; | ||
new SecureRandom().nextBytes(data); |
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.
nitpick: instead of generating random data we can stick with a predefined constant to avoid flakiness and make the test reproducible.
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.
This code is never actually called -- we only verify the generated module source code. But I'll change this to have a Random
instance as a constructor argument, as it's likely that host modules will have instance fields in practice.
Not sure what you mean, can you elaborate? |
d9a84dc
to
d9af6c1
Compare
Sure, sorry, I forgot to give context. |
Ah, I understand what you mean. I don't think those are an issue for arguments to host functions, since the memory will be already allocated. Returning strings from a host function should be the same problem as passing strings to an export function. We would need to know
|
The
@Buffer
annotation assumes aPOINTER_LENGTH
layout, which seems common. If needed, we can add an enum to support aLENGTH_POINTER
layout, with the default being the former.The
@Buffer
annotation could also be extended to supportbyte[]
, additional character sets for strings, etc.While there is lots more that we could do with layouts, this seems like a good step to handle a common case. It makes the logging for WASI significantly more useable, as we now have strings instead of pointers. This would allow us to add automatic logging/tracing support to the binding generator.