Skip to content

match the digit separator independently of char signedness - #336

Open
Ramya-9353 wants to merge 1 commit into
google:masterfrom
Ramya-9353:nonascii-separator-code-unit
Open

Ramya-9353 wants to merge 1 commit into
google:masterfrom
Ramya-9353:nonascii-separator-code-unit

Conversation

@Ramya-9353

Copy link
Copy Markdown
Contributor

Repro: with separator U+00A0 (the no-break space used as a thousands separator), the byte string "1\xA0" "000" parses as 1000 with 5 characters processed under -funsigned-char (the AArch64, PowerPC and s390x Linux ABIs) but is junk_string_value on a signed-char target; the uc16 overloads parse the equivalent string as 1000 everywhere.

Cause: Advance tests **it == separator, so the input byte is promoted through the platform's char. Where char is signed, a byte 0x80-0xFF sign-extends to a negative int and can never equal a separator above 0x7F.

Fix: compare the input through the CodeUnit helper that symbol matching already uses (#300), so the byte path matches the uc16 path on every target. Separators at or below 0x7F behave as before under either signedness, since a sign-extended byte never equals an ASCII code unit either way. This is the one remaining comparison of an input character that depends on the signedness of char; the classifiers audited in #324 only test ranges below 0x80.

The committed test uses the 0xA0 byte separator in integer and fractional positions. It fails on the current tree wherever char is signed (all CI runners) and passes with this change under either signedness.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request updates string-to-double.cc to use CodeUnit(**it) instead of **it when matching separators. This prevents sign-extension issues for separators above 0x7F (such as U+00A0) that can occur depending on the signedness of char. A new test StringToDoubleNonAsciiSeparator has also been added to verify this behavior. I have no feedback to provide.

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.

1 participant