-
Notifications
You must be signed in to change notification settings - Fork 212
Marshal String Encodings #1283
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?
Marshal String Encodings #1283
Conversation
|
|
||
| <file name = "src/cpp/encoding/Ascii.cpp"/> | ||
| <file name = "src/cpp/encoding/Utf8.cpp"/> | ||
| <file name = "src/cpp/encoding/Utf16.cpp"/> |
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 think it would be better to include these in the build only if the types are actually needed by haxe. They are more like utilities rather than something that should be a mandatory part of the runtime group.
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 look into it. Another thing I was thinking of was changing string.cpp to use these functions as well since there's a fair bit of duplication here, plus the functions in these classes deal with some things the string.cpp ones don't (various utf-8 ranges are invalid).
I need to check the performance characteristics though, since all these functions are bound checked whereas string.cpp works on raw pointers.
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 set it up to only be included if used but then remembered that I updated haxe.io.Bytes to use these new classes to avoid issues with needing to trim null terminating characters. Since they're used by a fairly central haxe type and will almost always be included I'm not sure it's worth the extra faff, so I reverted the changes.
I did a bit of benchmarking and converting 10 lorem ipsum paragraphs with some unicode characters from UTF16 to UTF8 100,000 times takes utf8_str 1.2s and cpp.encode.Utf8 2.9s. Some quick profiling showed that bounds checking is not the culprit, but I'll save resolving that difference for another time.
This reverts commit 695e134.
No changes were made in this area, dev haxe + hxcpp also fails to build that test locally for me
|
I have no idea whats going on with the 4.3 native mac and linux tests, there have been no changes to |
details on haxe side : HaxeFoundation/haxe#12476