-
Notifications
You must be signed in to change notification settings - Fork 758
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Don't optimize string literals to string.const
if they are not valid UTF-16 strings.
#7324
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.
lgtm otherwise, thanks! And sorry for the multiple iterations here.
@@ -991,6 +958,29 @@ struct Precompute | |||
return true; | |||
} | |||
|
|||
bool isValidUTF16Literal(const Literal& value) { |
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 the best place for this might be
src/support/string.h, cpp
alongside the existing isUTF8
.
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.
How do I go about the Literal
representation and std::string_view
or std:string
? I assume we don't want isValidUtf16Literal
taking a Literal
in string.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.
Hmm, we have related logic here:
Lines 660 to 677 in 5f767b7
case HeapType::string: { | |
auto data = literal.getGCData(); | |
if (!data) { | |
o << "nullstring"; | |
} else { | |
o << "string("; | |
// Convert WTF-16 literals to WTF-16 string. | |
std::stringstream wtf16; | |
for (auto c : data->values) { | |
auto u = c.getInteger(); | |
assert(u < 0x10000); | |
wtf16 << uint8_t(u & 0xFF); | |
wtf16 << uint8_t(u >> 8); | |
} | |
// Escape to ensure we have valid unicode output and to make | |
// unprintable characters visible. | |
// TODO: Use wtf16.view() once we have C++20. | |
String::printEscapedJSON(o, wtf16.str()); |
Perhaps in this PR we can leave it as is and just add a TODO, something like
// TODO: move this logic to src/support/string, and refactor to share code with wasm/literal.cpp string printing's conversion from a Literal to a raw string
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.
sg. Added a TODO. Thanks.
No description provided.