-
-
Notifications
You must be signed in to change notification settings - Fork 229
Wasm registration fn names now based on crate name + index #1205
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
base: master
Are you sure you want to change the base?
Conversation
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1205 |
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.
Thanks a lot, great simplification! 👍
Is the libc
dependency still necessary after this?
d1e47eb
to
e5ac8cc
Compare
In principle this should be fine, though we may want to go back to the hash idea if we hit some name length limit. Let's wait and see... |
3afea1b
to
9a4d537
Compare
Yeah, it's just that emscripten is now a bit picky about identifiers: #438 (comment) But it's causing Rust to not work altogether, so this is probably not a concern for us, but for them. :p |
This is ready from my side |
On my limited testing on hello-gdext-wasm, seems to work fine too. I'll just note that there are some concerns noted here: https://users.rust-lang.org/t/simple-state-in-procedural-macro/68204/2 In particular, proc macro caching and other details could potentially lead to problems. That thread also suggested using UUIDs, but I'm fairly sure they are generated using the current time? |
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'll just note that there are some concerns noted here: https://users.rust-lang.org/t/simple-state-in-procedural-macro/68204/2
Very good point 👍
@0x53A looks good for now. Could you apply my suggestion on the static
comment (verbatim), so we're not oblivious to this issue, should it cause trouble at some point?
add comment Co-authored-by: Jan Haller <[email protected]>
if it comes to that we can pass through an identifier so it would become |
fixes #1202