Implement round-half-even float.fromhex in common float_ops - #8234
Conversation
Add float_ops::from_hex returning Result<f64, HexFloatError>, which parses the coefficient and exponent, rounds round-half-even over the full exponent range, and detects overflow. Route float.fromhex through it: raise OverflowError for values too large to represent and ValueError for invalid input. Remove the expectedFailure on test_from_hex. Assisted-by: Claude
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds Python-style hexadecimal float parsing with dedicated error mapping, shared ASCII whitespace handling, parser tests, and two spelling dictionary entries. ChangesHex Float Parsing
Spelling Dictionary Update
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.1).cspell.jsonFile contains syntax errors that prevent linting: Line 1: Expected an array, an object, or a literal but instead found '// See: https://github.com/streetsidesoftware/cspell/tree/; Line 6: Expected an array, an object, or a literal but instead found '// " ... [truncated 1263 characters] ... ted; Line 98: End of file expected; Line 99: End of file expected; Line 99: End of file expected; Line 100: End of file expected; Line 100: End of file expected; Line 101: End of file expected; Line 101: End of file expected; Line 102: End of file expected; Line 102: End of file expected; Line 103: End of file expected; Line 103: End of file expected; Line 105: End of file expected; Line 106: End of file expected; Line 108: End of file expected; Line 108: End of file expected; Line 108: End of file expected; Line 109: End of file expected; Line 110: End of file expected; Line 110: End of file expected; Line 110: End of file expected; Line 111: End of file expected; Line 113: End of file expected; Line 113: End of file expected; Line 113: End of file expected; Line 119: End of file expected 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/common/src/float_ops.rs (1)
503-515: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
is_ascii_digit()hereClippy will flag this manual digit test;
hex_byte_at(bytes, idx).is_ascii_digit()is clearer and removes the duplicate lookup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/common/src/float_ops.rs` around lines 503 - 515, The hex float exponent parsing in float_ops::hex_byte_at/exp handling uses a manual byte range check that Clippy flags and repeats the same lookup. Update the digit validation in this block to use the byte’s is_ascii_digit() method instead, and keep the surrounding exp_start/index advancement logic unchanged so the parsing behavior stays the same.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/common/src/float_ops.rs`:
- Around line 503-515: The hex float exponent parsing in
float_ops::hex_byte_at/exp handling uses a manual byte range check that Clippy
flags and repeats the same lookup. Update the digit validation in this block to
use the byte’s is_ascii_digit() method instead, and keep the surrounding
exp_start/index advancement logic unchanged so the parsing behavior stays the
same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: c4465655-ccab-4da5-bcd7-babda2b29650
⛔ Files ignored due to path filters (1)
Lib/test/test_float.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/common/src/float_ops.rscrates/vm/src/builtins/float.rs
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] test: cpython/Lib/test/test_float.py (TODO: 3) dependencies: dependent tests: (no tests depend on float) Legend:
|
…prefix Assisted-by: Claude
|
Thanks for the review — addressed in 51b3c1a:
— commented by Claude |
Assisted-by: Claude
… sre_engine Assisted-by: Claude
|
Update on the whitespace helper (after some more thought on where it belongs): instead of For the record, on two related questions that came up: the hex-float parser can't be borrowed from — commented by Claude |
Remove the private copy of is_py_ascii_whitespace in bytes_inner.rs and import the shared rustpython_common::wtf8::is_py_ascii_whitespace. Assisted-by: Claude
ShaharNaveh
left a comment
There was a problem hiding this comment.
thanks for addressing the review:)
|
@ShaharNaveh Thanks! I forgot to draft the first version |
Summary
float.fromhexdelegated toliteral::float::from_hex, which is backed byhexf_parse. That reader is meant for exact literals: it rejects any inexact hexadecimal input and cannot tell overflow apart from a parse error, sofloat.fromhexcould not round and never raisedOverflowError.test_from_hexwas consequently marked@unittest.expectedFailure.This adds a dedicated hex-float reader in
common::float_opsthat reproduces the coefficient/exponent parsing, round-half-even rounding, and overflow detection thatfloat.fromhexrequires, and routes the classmethod through it.Changes
common::float_ops::from_hex(&str) -> Result<f64, HexFloatError>: round-half-even over the full exponent range (subnormals down to0x1p-1074), theinf/nanspellings, surrounding ASCII whitespace, a saturating exponent parse, andInvalid/TooLong/Overflowerror kinds.float.fromhexnow mapsOverflow → OverflowErrorandInvalid/TooLong → ValueError, with the messages Python uses.@unittest.expectedFailurefromtest_from_hex.inf/nansign bits, whitespace, and the error kinds.Verification
from_hexoutput was compared bit-for-bit against CPython'sfloat.fromhexover 125,000 inputs (curated + fuzzed, including near-halfway rounding and theDBL_MAXboundary): 0 mismatches.cargo test -p rustpython-commonandpython -m test test_floatpass, withtest_from_hexnow green.— commented by Claude
Summary by CodeRabbit
float.fromhex()now accepts more Python-style hex float inputs, includinginf/nan, optional signs, whitespace, and full binary exponent parsing.