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.

Sign up for GitHub

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

Jump to bottom

feat(ecmascript): String fromCodePoints #543

Merged
merged 10 commits into from
Feb 1, 2025
Merged

feat(ecmascript): String fromCodePoints #543

merged 10 commits into from
Feb 1, 2025
+83 −33

Conversation

yossydev
Copy link
Contributor

@yossydev yossydev force-pushed the feat/string-code-points branch from a2e42a6 to 20b990a Compare January 19, 2025 07:31
@yossydev yossydev force-pushed the feat/string-code-points branch from 20b990a to 2cbd76a Compare January 22, 2025 13:59
@yossydev yossydev requested a review from aapoalas January 22, 2025 14:04
aapoalas
aapoalas requested changes Jan 26, 2025
View reviewed changes
Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

Found some differences compared to what the spec expects.

nova_vm/src/ecmascript/builtins/text_processing/string_objects/string_constructor.rs Outdated Show resolved Hide resolved
nova_vm/src/ecmascript/builtins/text_processing/string_objects/string_constructor.rs Outdated Show resolved Hide resolved
nova_vm/src/ecmascript/builtins/text_processing/string_objects/string_constructor.rs Outdated Show resolved Hide resolved
nova_vm/src/ecmascript/builtins/text_processing/string_objects/string_constructor.rs Outdated Show resolved Hide resolved
@yossydev yossydev force-pushed the feat/string-code-points branch from b30cda7 to bc46921 Compare January 29, 2025 14:18
@yossydev
Copy link
Contributor Author

@aapoalas
Thank you for the review! I really appreciate it!
I’m not sure if my understanding is correct, but I made the fix with bc46921

I have two things I’d like to confirm.

First, in the fast path, I’m using into_i64, but in the non-fast path, I’m using to_uint32_number.
Did you originally intend for into_i64 to be used in the latter case as well? I initially tried using into_i64, but it caused an error in tests/test262/test/built-ins/String/fromCodePoint/number-is-out-of-range.js when -1 was included, so I decided to use to_uint32_number instead.

Second, in the fast path, if I remove to_number, I can’t use into_i64, so I left it as is.
However, is_integral_number seemed fine to remove, so I went ahead and removed it!

@yossydev yossydev requested a review from aapoalas January 29, 2025 14:26
aapoalas
aapoalas requested changes Jan 29, 2025
View reviewed changes
Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

Still a few problems, sorry :( But after these I think this should be good and very solid <3

nova_vm/src/ecmascript/builtins/text_processing/string_objects/string_constructor.rs Outdated Show resolved Hide resolved
nova_vm/src/ecmascript/builtins/text_processing/string_objects/string_constructor.rs Outdated Show resolved Hide resolved
nova_vm/src/ecmascript/builtins/text_processing/string_objects/string_constructor.rs Outdated Show resolved Hide resolved
nova_vm/src/ecmascript/builtins/text_processing/string_objects/string_constructor.rs Outdated Show resolved Hide resolved
nova_vm/src/ecmascript/builtins/text_processing/string_objects/string_constructor.rs Outdated Show resolved Hide resolved
@yossydev yossydev force-pushed the feat/string-code-points branch from bc46921 to 8e63517 Compare January 30, 2025 13:56
@yossydev
Copy link
Contributor Author

Still a few problems, sorry :( But after these I think this should be good and very solid <3

@aapoalas
No, thank you for reviewing it multiple times! I really appreciate it!!
I’ve fixed it here: 8e63517ac746032dc4e1692cd2ba2a132b1fa49b!!

@yossydev yossydev requested a review from aapoalas January 30, 2025 14:05
aapoalas
aapoalas requested changes Jan 30, 2025
View reviewed changes
Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

The generic path shouldn't assume Value::Integer.

nova_vm/src/ecmascript/builtins/text_processing/string_objects/string_constructor.rs Show resolved Hide resolved
nova_vm/src/ecmascript/builtins/text_processing/string_objects/string_constructor.rs Outdated Show resolved Hide resolved
nova_vm/src/ecmascript/builtins/text_processing/string_objects/string_constructor.rs Outdated Show resolved Hide resolved
@yossydev yossydev force-pushed the feat/string-code-points branch from c399d6a to 147cb9b Compare January 31, 2025 00:32
@yossydev
Copy link
Contributor Author

@aapoalas
Thank you for the review! And I’m sorry for asking again, but I’ve made some corrections. Would you mind reviewing it once more? 🙇‍♂️ 147cb9b

@yossydev yossydev requested a review from aapoalas January 31, 2025 00:35
aapoalas
aapoalas previously approved these changes Feb 1, 2025
View reviewed changes
Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

LGTM, thank you! Had one minor nitpick which has no effect in the end :)

nova_vm/src/ecmascript/builtins/text_processing/string_objects/string_constructor.rs Outdated Show resolved Hide resolved
@aapoalas
Copy link
Collaborator

aapoalas commented Feb 1, 2025

BTW you seem to have updated the test262 submodule at some point. Or maybe I did that?

Anyway, it's not the best thing to do it in this PR but it's fine, let's keep it :)

@aapoalas aapoalas merged commit f2b0567 into trynova:main Feb 1, 2025
1 check passed
yossydev added a commit to yossydev/nova that referenced this pull request Feb 1, 2025
@yossydev
Revert "feat(ecmascript): String fromCodePoints (trynova#543)"
0d12e60 
This reverts commit f2b0567.
@yossydev
Copy link
Contributor Author

yossydev commented Feb 1, 2025

BTW you seem to have updated the test262 submodule at some point. Or maybe I did that?

Anyway, it's not the best thing to do it in this PR but it's fine, let's keep it :)

Thank you for merging! And sorry, I didn’t mean to update it, but it looks like it got included in the changes! 🙇‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aapoalas aapoalas aapoalas left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yossydev @aapoalas