Skip to content

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Mar 27, 2023

Followup to #19064

All remaining internal usages converted to allocateUTF8. These functions do the same thing and were made into aliases in #19064.

stringToNewUTF8 was added in 443447b and has fewer uses than allocateUTF8.

allocateUTF8 was added in 5174d2e.

@sbc100 sbc100 force-pushed the use_allocate_utf8 branch from 8a08504 to fba10b7 Compare March 27, 2023 21:37
Base automatically changed from use_allocate_utf8 to main March 27, 2023 22:44
All remaining internal usages converted to `allocateUTF8`.  These
functions do that same thing and were made into aliases in #19064.

`stringToNewUTF8` was added in 443447b and has fewer uses than
`allocateUTF8`.

`allocateUTF8` was added in 5174d2e.
@sbc100 sbc100 force-pushed the deprecate_stringToNewUTF8 branch from 110b21f to 1076c01 Compare March 27, 2023 23:25
@sbc100 sbc100 requested review from juj and tlively March 27, 2023 23:25
@juj
Copy link
Collaborator

juj commented Mar 28, 2023

Originally I created the function pair stringToUTF8 and stringToNewUTF8 as a symmetric API for marshalling a string from JS to already allocated memory area in wasm (e.g. onto stack), or marshalling a string to a new dynamically allocated memory area in wasm (onto heap).

The function name allocateUTF8 felt ambiguous/lacking to me, it read like a malloc function, but it actually not just allocates space for a string, but also copies a string over to wasm as well.

I am not opposed to deprecating stringToNewUTF8 in favor of allocateUTF8 if the new consensus is strongly towards the opposite?

However it will leave the API asymmetric: stringToUTF8 to marshal an existing string over, and allocateUTF8 to dynamically allocate a new UTF8 string to the wasm heap. Is that a non-concern?

In our codebase we do not currently use allocateUTF8 anywhere, but the stringToUTF8 and stringToNewUTF8 pair.

@sbc100
Copy link
Collaborator Author

sbc100 commented Mar 28, 2023

I'm happy to reverse this and deprecate allocateUTF8 instead. I just think one of them should deprecated, I don't really care which.

@juj
Copy link
Collaborator

juj commented Mar 28, 2023

I think it would seem somewhat off to call stringToNewUTF8 deprecated, but stringToUTF8 not. They feel quite hand in hand?

Would it be beneficial for more symmetry to keep the stringToNewUTF8 and stringToUTF8 pair over allocateUTF8?

@sbc100 sbc100 closed this Mar 28, 2023
@sbc100 sbc100 deleted the deprecate_stringToNewUTF8 branch March 28, 2023 18:53
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

Successfully merging this pull request may close these issues.

4 participants