-
-
Notifications
You must be signed in to change notification settings - Fork 414
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
Refactor RawJsString
's representation to make JsString
s construction from string literal heap-allocation free
#3935
Refactor RawJsString
's representation to make JsString
s construction from string literal heap-allocation free
#3935
Conversation
union
to represent RawJsString
RawJsString
to make JsString
created from string literal without heap allocation
RawJsString
to make JsString
created from string literal without heap allocationRawJsString
's representaion to make JsString
s construction from string literal heap-allocation
free
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3935 +/- ##
==========================================
+ Coverage 47.24% 51.57% +4.33%
==========================================
Files 476 468 -8
Lines 46892 44983 -1909
==========================================
+ Hits 22154 23202 +1048
+ Misses 24738 21781 -2957 ☔ View full report in Codecov by Sentry. |
RawJsString
's representaion to make JsString
s construction from string literal heap-allocation
freeRawJsString
's representation to make JsString
s construction from string literal heap-allocation
free
RawJsString
's representation to make JsString
s construction from string literal heap-allocation
freeRawJsString
's representation to make JsString
s construction from string literal heap-allocation free
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 is looking fantastic overall! Thanks for all the effort you put into this!
I still want to play around with it a bit more in depth, but thought I'd leave a couple comments.
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.
Implementation wise looks perfect! I just have some suggestions around some safety comments that could be improved.
@jedel1043 , I've improved the comment, if you don't think it's good enough, please continue to point it out. |
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.
Overall, this PR looks fantastic. Great work on this!
I saw the test added, but I still find myself wishing there were more tests. I'm not entirely sure that's not necessary to merge, imo. Maybe it would be a good follow up or first issue for someone? The docs improvements are also really nice as well.
As a separate note, @jedel1043 this may be a good candidate to build out design docs for per #3938.
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.
Looks perfect to me!
@nekevss Yeah, I'll open an issue for general improvements around documenting the current design of JsString
.
This changes
RawJsString
's representation,RawJsString
inJsString
is now a memory variant ofRawJsString
andStaticJsString
which holds a string literal. It has a few benefits we can gain:miri
, thanks to @jedel1043 )JsString
could be constant nowJsString
is created from string literal by usingjs_string
macro