Repository navigation
Read decimal digit values from the bundled character database - #8748
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: RustPython/RustPython/.coderabbit.yml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthrough
ChangesUnicode decimal lookup
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Feature Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
6c5d118 to
d9ede6d
Compare
`char_to_decimal` carried its own sorted table of 590 decimal digit characters, added in RustPython#5643 because `common` had no way to reach the Unicode numeric data at the time: it lived in `stdlib`, which sits above `common` in the dependency graph. The table's own comment said as much — "this should probably live in a crate like unic or unicode-properties". RustPython#8211 extracted that data into `rustpython-unicode`, below `common`, and made `common` depend on it, so the table can now be dropped in favour of `Ucd::decimal` — the same source `unicodedata.decimal` consults. This fixes a CPython incompatibility. The table was last updated for Unicode 10; the bundled UCD is Unicode 17, which has 180 more decimal digits across 18 scripts (Hanifi Rohingya, Wancho, Tangsa, Ol Onal, segmented digits and others). Those were rejected by `int()` and `float()` even though `unicodedata.decimal` knew their value: >>> int('\u{10D33}') # HANIFI ROHINGYA DIGIT THREE ValueError # CPython: 3 >>> unicodedata.decimal('\u{10D33}') 3 All 590 characters the table covered keep their previous value, so nothing that already worked changes, and the two lookups can no longer drift apart on a future Unicode update. The lookup costs about 1.5ns more per non-ASCII character, since it reads a property trie and a value table instead of binary-searching one 2.4KB array: `int()` over a 4000-digit non-ASCII string goes from 12.2 to 13.6 ns/char. ASCII input never reaches it — the sole caller, `transform_decimal_and_space_to_ascii`, returns early for ASCII strings — so ordinary numeric parsing is unaffected. Assisted-by: Claude Code:claude-opus-5
d9ede6d to
18b9b5b
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
youknowone
left a comment
There was a problem hiding this comment.
you caught a good point, thanks!
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
One of checkbox below must be checked.
Summary
char_to_decimalincrates/common/src/str.rscarried its own sorted table of 590 decimal digit characters. It was added in #5643 becausecommonhad no way to reach the Unicode numeric data at the time — that data lived instdlib, which sits abovecommonin the dependency graph. The table's own comment said as much:// TODO: this should probably live in a crate like unic or unicode-properties#8211 extracted that data into
rustpython-unicode, which sits belowcommon, and madecommondepend on it. So the table can now go, in favour ofUcd::decimal— the same sourceunicodedata.decimalconsults.This fixes a CPython incompatibility
The hand-written table was last updated for Unicode 10. The bundled UCD is Unicode 17, which has 180 more decimal digits across 18 scripts — Hanifi Rohingya, Wancho, Tangsa, Ol Onal, Kirat Rai, segmented digits and others.
int()andfloat()rejected all of them, even thoughunicodedata.decimalin the same interpreter knew their value:CPython accepts
int('𐴳')and returns3. After this patch RustPython does too, and the two lookups can no longer drift apart on a future Unicode update.All 590 characters the old table covered keep their previous value (verified by diffing the table against the bundled UCD: 180 gained, 0 changed, 0 lost), so nothing that already worked changes.
Performance
About 1.5ns more per non-ASCII character, since it reads a property trie and a value table rather than binary-searching one 2.4KB array —
int()over a 4000-digit non-ASCII string goes from 12.2 to 13.6 ns/char. ASCII input never reaches it: the sole caller,transform_decimal_and_space_to_ascii, returns early for ASCII strings, so ordinary numeric parsing is unaffected (5.6 → 5.4 ns/char, i.e. noise).Tests
Adds
char_to_decimal_values, covering ASCII, an Arabic-Indic digit the old table had, two digits it predated (U+10D35, U+1FBF5), and non-decimal characters (½,가).🤖 Generated with Claude Code