Skip to content

Conversation

@scottp-dpaw
Copy link
Contributor

No description provided.

@necouchman
Copy link
Contributor

@scottp-dpaw Can you please re-base this against the patch branch of the Apache repository, instead of main?

@scottp-dpaw scottp-dpaw changed the base branch from main to patch July 30, 2024 03:17
@scottp-dpaw
Copy link
Contributor Author

Done, sorry about that!

@scottp-dpaw
Copy link
Contributor Author

Fixed. I added a guard for String.fromCodePoint as well, given it's the same age as String.codePointAt

@necouchman necouchman requested a review from jmuehlner August 20, 2025 12:42
Copy link
Contributor

@necouchman necouchman left a comment

Choose a reason for hiding this comment

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

Thanks, @scottp-dpaw, looks good to me.

@jmuehlner or @mike-jumper, any concerns?

Copy link
Contributor

@mike-jumper mike-jumper left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me - just a couple issues:

  • Comparisons of integer values should definitely use !== instead of !=.
  • There's a wonky null statement that crept in here - just accidentally double up on the semicolons.

var codepoint = str.codePointAt ? str.codePointAt(i) : str.charCodeAt(i);

// For surrogate pairs, skip the second 16 bits.
if (str.charCodeAt(i) != codepoint) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This comparison should use !== (same elsewhere).

// Fill buffer with UTF-8
for (var i=0; i<text.length; i++) {
var codepoint = text.charCodeAt(i);
var codepoint = text.codePointAt ? text.codePointAt(i) : text.charCodeAt(i);;
Copy link
Contributor

Choose a reason for hiding this comment

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

Beware you accidentally added an extra semicolon here (null statement).

@mike-jumper mike-jumper changed the base branch from patch to staging/1.6.1 September 30, 2025 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants