Skip to content

Optimize JsStr::code_points() for Latin1 strings#4553

Merged
jedel1043 merged 2 commits intoboa-dev:mainfrom
Abd002:main
Dec 2, 2025
Merged

Optimize JsStr::code_points() for Latin1 strings#4553
jedel1043 merged 2 commits intoboa-dev:mainfrom
Abd002:main

Conversation

@Abd002
Copy link
Copy Markdown
Contributor

@Abd002 Abd002 commented Nov 30, 2025

This Pull Request optimizes the code_points() method for Latin1 strings.

It changes the following:

  • Added CodePointIter enum with specialized Latin1 path
  • Latin1 strings now avoid UTF-16 conversion overhead
  • UTF-16 string behavior remains unchanged
  • Added tests for both encoding paths

@github-actions
Copy link
Copy Markdown

Test262 conformance changes

Test result main count PR count difference
Total 50,747 50,747 0
Passed 47,877 47,877 0
Ignored 2,060 2,060 0
Failed 810 810 0
Panics 0 0 0
Conformance 94.34% 94.34% 0.00%

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 1, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.14%. Comparing base (6ddc2b4) to head (5de47ab).
⚠️ Report is 608 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4553      +/-   ##
==========================================
+ Coverage   47.24%   57.14%   +9.90%     
==========================================
  Files         476      504      +28     
  Lines       46892    57558   +10666     
==========================================
+ Hits        22154    32893   +10739     
+ Misses      24738    24665      -73     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@HalidOdat HalidOdat requested a review from a team December 1, 2025 14:43
Copy link
Copy Markdown
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

I have a small suggestion about the new test. Everything else looks good!

@jedel1043 jedel1043 added A-Enhancement New feature or request A-API Changes related to public APIs labels Dec 1, 2025
@jedel1043 jedel1043 added this to the v1.0.0 milestone Dec 1, 2025
@Abd002 Abd002 requested a review from jedel1043 December 1, 2025 19:36
Copy link
Copy Markdown
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

Looks good to me! 😄

@HalidOdat HalidOdat enabled auto-merge December 1, 2025 21:21
Copy link
Copy Markdown
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Final round of comments and we can review after that. Thank you for the contribution!

@jedel1043 jedel1043 disabled auto-merge December 1, 2025 21:37
@jedel1043
Copy link
Copy Markdown
Member

Ah, you may have to add "Caf" to the list of excluded words in typos.toml.

- Add CodePointIter with specialized Latin1 path
- Latin1 strings avoid UTF-16 conversion overhead
- UTF-16 behavior remains unchanged
- Add tests for both encoding paths
- Include Latin1-compatible characters in tests
@Abd002 Abd002 requested a review from jedel1043 December 2, 2025 03:57
Copy link
Copy Markdown
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

LGTM!

@jedel1043 jedel1043 enabled auto-merge December 2, 2025 04:14
@jedel1043 jedel1043 added this pull request to the merge queue Dec 2, 2025
Merged via the queue into boa-dev:main with commit dfee7ad Dec 2, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-API Changes related to public APIs A-Enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants