Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate JsString into its own crate #3837

Merged
merged 1 commit into from May 12, 2024
Merged

Separate JsString into its own crate #3837

merged 1 commit into from May 12, 2024

Conversation

HalidOdat
Copy link
Member

This Pull Request is a continuation of the work on #3829 , by moving the string crate because the ByteCompiler depends on it.

There are still some issues with js_str! macro that constructs a JsStr, there is no $crate for proc macros unfortunately 馃槩 Fixed by making it point to boa_engine otherwise you have to include boa_string everywhere... If there is another solution for this, would love to hear it :)

@HalidOdat HalidOdat added enhancement New feature or request Internal Category for changelog and removed enhancement New feature or request labels May 2, 2024
@HalidOdat HalidOdat added this to the v0.18.1 milestone May 2, 2024
Copy link

github-actions bot commented May 2, 2024

Test262 conformance changes

Test result main count PR count difference
Total 50,731 50,731 0
Passed 42,973 42,973 0
Ignored 1,395 1,395 0
Failed 6,363 6,363 0
Panics 18 18 0
Conformance 84.71% 84.71% 0.00%

@HalidOdat

This comment was marked as resolved.

Copy link
Member

@raskad raskad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work :)

core/engine/src/string.rs Outdated Show resolved Hide resolved
core/engine/src/string.rs Outdated Show resolved Hide resolved
Copy link
Member

@nekevss nekevss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just had one passing thought when reviewing, but nothing blocking for me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: should we look to move Tagged<T> into a utility crate.

Obviously this is super minor since it's really only like 100 lines, but instead of having this reimplemented in boa_string, should we look to move tag into a utility crate and then import it into boa_engine and boa_string?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was going to do that, but wanted to keep the PR small. Will do that in a follow up PR :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the PR for it: #3849 :)

@HalidOdat HalidOdat added this pull request to the merge queue May 12, 2024
Merged via the queue into main with commit ced222f May 12, 2024
13 checks passed
@jedel1043 jedel1043 deleted the js-string-crate branch May 12, 2024 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Category for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants