feat(dfloat): portable blockbinary storage, string I/O, and formatting#524
Merged
Ravenwater merged 6 commits intomainfrom Feb 28, 2026
Merged
feat(dfloat): portable blockbinary storage, string I/O, and formatting#524Ravenwater merged 6 commits intomainfrom
Ravenwater merged 6 commits intomainfrom
Conversation
…kbinary<> Replaces bt _block[nrBlocks] with blockbinary<nbits> for encoding storage and replaces the platform-specific __uint128_t significand_t with blockbinary<4*ndigits+8> for significand arithmetic. This eliminates all 14 __SIZEOF_INT128__ guards, making decimal128 (dfloat<34,12>) fully portable to MSVC and extensible to arbitrary precision beyond 38 digits. Unified all dual code paths in operator+=, operator*=, operator/=, pack/unpack, and DPD encode/decode into single implementations that work uniformly for any ndigits via blockbinary's arbitrary-width arithmetic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
str() now respects scientific, fixed, and automatic format modes with precision control. In automatic mode, scientific notation is used when the decimal exponent exceeds the type's digit count, preventing enormous strings for extreme values like numeric_limits::max() and min(). operator<< extracts std::scientific/std::fixed/setw flags from the stream and delegates to str() accordingly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…le cast dynamic_range() was casting dfloat values to double before printing, causing overflow to inf for decimal64/128 maxpos (scale 384/6144 exceeds double max ~1.8e308) and underflow to 0 for their minpos values. Print dfloat values directly via operator<< to preserve the full decimal dynamic range. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…t to ndigits precision
Implement assign(const std::string&) to parse decimal strings in fixed
("999.9999"), scientific ("-123.456e-78"), integer ("42"), fractional
(".5"), and special value ("inf", "nan") formats. Parsing is exact with
no binary conversion — digits are collected directly into the blockbinary
significand_t and packed with the decimal exponent.
Change operator<< to default to ndigits precision instead of the iostream
default of 6, since all stored decimal digits are exact and should be
shown. Stream precision is only applied when the user explicitly sets
std::scientific or std::fixed mode.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR refactors the dfloat decimal floating-point type's internal storage mechanism from platform-specific native integers to blockbinary-based storage, removes conditional compilation guards for 128-bit decimal types, updates test infrastructure with explicit type instantiations, and fixes normalization control flow in hfloat arithmetic operations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
playground/regression_test_skeleton.cpp (1)
1-1: Tighten skeleton labeling for clarity.Line 1 and Line 35 frame this as “areal addition,” but the file currently provides generic scaffolding without concrete addition test calls. Consider renaming strings/comments to “regression skeleton” until test routes are filled in.
Also applies to: 35-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playground/regression_test_skeleton.cpp` at line 1, The header/comment labels in regression_test_skeleton.cpp currently advertise "areal addition" but the file is a generic test scaffold; change any user-facing strings and comments that say "areal addition" (e.g., the top comment and the label at line 35) to a neutral name like "regression skeleton" or "regression test skeleton" so the file accurately reflects its placeholder status until concrete addition test calls are implemented.static/float/dfloat/api/ranges.cpp (1)
18-21: Unused variabletest_tag.Same as in
api.cpp, thetest_tagvariable is declared but never used.🔧 Suggested fix
std::string test_suite = "dfloat<> range tests"; - std::string test_tag = "dfloat<> ranges"; bool reportTestCases = false; int nrOfFailedTestCases = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@static/float/dfloat/api/ranges.cpp` around lines 18 - 21, The variable test_tag declared alongside test_suite, reportTestCases and nrOfFailedTestCases is unused; remove the unused declaration of test_tag from the top of ranges.cpp (or, if intended to be used, actually use it in the test harness where other tags are referenced) so the compiler warning is removed—look for the declaration "std::string test_tag" and either delete that line or replace its use consistent with how test_suite/reportTestCases are used in the surrounding test code.static/float/dfloat/api/api.cpp (2)
167-172: Consider adding an assertion for the decimal exactness test.The decimal exactness test demonstrates that
10 * 0.1 = 1.0exactly in decimal floating-point, but doesn't verify this programmatically. Adding a check would make this a proper regression test.🧪 Suggested enhancement
// accumulate ten times 0.1 - should be exactly 1.0 Real sum(0); for (int i = 0; i < 10; ++i) sum += tenth; std::cout << "10 * 0.1 = " << sum << '\n'; + if (sum != Real(1)) { + std::cerr << "FAIL: 10 * 0.1 should equal exactly 1.0 in decimal float\n"; + ++nrOfFailedTestCases; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@static/float/dfloat/api/api.cpp` around lines 167 - 172, Add a programmatic assertion after the decimal exactness loop to verify the result equals 1.0: after accumulating into Real sum and printing it (in api.cpp within the same scope where Real sum and tenth are used), add a check such as assert(sum == Real(1)) (or use an equivalent test that reports failure) so the test fails automatically on regression; ensure you include <cassert> or use the project’s existing test/assert mechanism consistent with other tests.
55-58: Unused variabletest_tag.The variable
test_tagis declared but never used in the function. Consider removing it or using it in the test suite reporting.🔧 Suggested fix
std::string test_suite = "dfloat<> Application Programming Interface tests"; - std::string test_tag = "dfloat<> API"; bool reportTestCases = false; int nrOfFailedTestCases = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@static/float/dfloat/api/api.cpp` around lines 55 - 58, The variable test_tag is declared but never used; remove the unused declaration of test_tag from the function (leaving test_suite, reportTestCases, and nrOfFailedTestCases intact), or if you prefer to keep a tag, use test_tag where test-suite reporting occurs (e.g., include it in any log/report call that references test_suite or reportTestCases) so the symbol test_tag is actually referenced; ensure there are no remaining references to the removed symbol.include/sw/universal/number/dfloat/dfloat_impl.hpp (2)
148-153: Consider caching or binary exponentiation forpow10_s.The current implementation computes
10^nvianmultiplications. For decimal128 with 34 digits, this means up to 33 multiplications per call. Sincepow10_sis called frequently (in pack/unpack, normalization, arithmetic), consider caching commonly-used powers or using binary exponentiation.This is a low-priority optimization since the current implementation is correct and the loop count is bounded by
ndigits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/sw/universal/number/dfloat/dfloat_impl.hpp` around lines 148 - 153, The pow10_s function currently computes 10^n by n multiplications; replace it with a faster approach by either (a) adding a static constexpr cache/lookup table of precomputed powers of 10 (e.g., an array of significand_t entries for 10^0..10^(ndigits-1)) and returning cache[n], or (b) implementing binary exponentiation (exponentiation by squaring) inside pow10_s to reduce the number of multiplications; update function pow10_s to use the chosen method and ensure thread-safe initialization if you use a runtime cache.
1026-1035: Conditionsig_bits <= 64excludes decimal64 from direct conversion.With
sig_bits = 4*ndigits + 8, decimal64 (ndigits=16) hassig_bits = 72 > 64, so it uses the string conversion path. This works correctly but is slightly less efficient than necessary, since a 16-digit decimal significand (max ~10^16) fits in uint64_t. Consider usingndigits <= 19as the threshold instead.⚡ Performance optimization
- if constexpr (sig_bits <= 64) { - sig_d = static_cast<double>(static_cast<unsigned long long>(sig)); + // uint64_t can hold up to 10^19-1, so direct conversion is safe for ndigits <= 19 + if constexpr (ndigits <= 19) { + sig_d = static_cast<double>(static_cast<unsigned long long>(sig));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/sw/universal/number/dfloat/dfloat_impl.hpp` around lines 1026 - 1035, The branch that decides using integer-to-double conversion currently checks "if constexpr (sig_bits <= 64)" which excludes decimal64 (ndigits=16) even though its significand fits in uint64_t; change the condition to test the digit count instead (e.g., "if constexpr (ndigits <= 19)") so 16-digit decimal64 uses direct static_cast conversion of sig to double; update the comment accordingly and keep the fallback using to_decimal(sig) for wider significands (references: sig_bits, ndigits, sig, sig_d, to_decimal).static/float/dfloat/standard/decimal64.cpp (2)
107-119: BID/DPD comparison via double conversion may mask precision differences.The test compares BID and DPD encodings by converting both to
double. While this works for the test values used, it could mask representation differences for values at the edge of double's precision (15-17 digits). For a more rigorous test, consider comparing the unpacked significand and exponent directly.For the current test values (small integers and simple decimals), the double comparison is adequate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@static/float/dfloat/standard/decimal64.cpp` around lines 107 - 119, The test currently compares BID and DPD by converting decimal64_bid and decimal64_dpd to double (variables decimal64_bid, decimal64_dpd and comparisons of dbid/ddpd), which can hide precision/exponent differences; change the test to unpack and compare the internal components (significand/mantissa and exponent/scale) of decimal64_bid and decimal64_dpd directly instead of casting to double—use the types' accessors or implement helper functions to extract the significand and exponent for decimal64_bid and decimal64_dpd, compare those fields and increment nrOfFailedTestCases and print via reportTestCases on mismatch (keeping the same fail reporting flow).
163-170: Empty regression levels 2-4.The higher regression levels are empty placeholders. Consider adding TODO comments or removing the empty blocks until tests are implemented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@static/float/dfloat/standard/decimal64.cpp` around lines 163 - 170, The preprocessor blocks for REGRESSION_LEVEL_2, REGRESSION_LEVEL_3, and REGRESSION_LEVEL_4 in decimal64.cpp are empty; either remove these empty `#if` ... `#endif` sections or add a short TODO comment inside each (e.g., "// TODO: implement regression tests for LEVEL_X") so the intent is clear and the placeholders aren't silent; locate the three blocks guarded by REGRESSION_LEVEL_2, REGRESSION_LEVEL_3, and REGRESSION_LEVEL_4 and update them accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@playground/regression_test_skeleton.cpp`:
- Around line 19-28: The source currently forces REGRESSION_LEVEL_2 and
REGRESSION_LEVEL_3 on (and overrides REGRESSION_LEVEL_4) which bypasses CMake
control; change the block guarded by REGRESSION_LEVEL_OVERRIDE so that only
REGRESSION_LEVEL_1 is defaulted to 1 (sanity), and do not enable or set
REGRESSION_LEVEL_2, REGRESSION_LEVEL_3, or REGRESSION_LEVEL_4 in-source; leave
their defines untouched so CMake can drive them (adjust the code around
REGRESSION_LEVEL_OVERRIDE, REGRESSION_LEVEL_1, REGRESSION_LEVEL_2,
REGRESSION_LEVEL_3, REGRESSION_LEVEL_4 accordingly).
In `@static/float/dfloat/api/ranges.cpp`:
- Line 1: The top-of-file header comment is incorrect (it reads "api.cpp") and
should be updated to match this file's purpose and name; replace or edit the
header comment at the top of ranges.cpp so it correctly identifies the file as
"ranges.cpp" and, if appropriate, briefly describes that it contains API tests
for decimal floating-point range operations to avoid the copy-paste mismatch.
---
Nitpick comments:
In `@include/sw/universal/number/dfloat/dfloat_impl.hpp`:
- Around line 148-153: The pow10_s function currently computes 10^n by n
multiplications; replace it with a faster approach by either (a) adding a static
constexpr cache/lookup table of precomputed powers of 10 (e.g., an array of
significand_t entries for 10^0..10^(ndigits-1)) and returning cache[n], or (b)
implementing binary exponentiation (exponentiation by squaring) inside pow10_s
to reduce the number of multiplications; update function pow10_s to use the
chosen method and ensure thread-safe initialization if you use a runtime cache.
- Around line 1026-1035: The branch that decides using integer-to-double
conversion currently checks "if constexpr (sig_bits <= 64)" which excludes
decimal64 (ndigits=16) even though its significand fits in uint64_t; change the
condition to test the digit count instead (e.g., "if constexpr (ndigits <= 19)")
so 16-digit decimal64 uses direct static_cast conversion of sig to double;
update the comment accordingly and keep the fallback using to_decimal(sig) for
wider significands (references: sig_bits, ndigits, sig, sig_d, to_decimal).
In `@playground/regression_test_skeleton.cpp`:
- Line 1: The header/comment labels in regression_test_skeleton.cpp currently
advertise "areal addition" but the file is a generic test scaffold; change any
user-facing strings and comments that say "areal addition" (e.g., the top
comment and the label at line 35) to a neutral name like "regression skeleton"
or "regression test skeleton" so the file accurately reflects its placeholder
status until concrete addition test calls are implemented.
In `@static/float/dfloat/api/api.cpp`:
- Around line 167-172: Add a programmatic assertion after the decimal exactness
loop to verify the result equals 1.0: after accumulating into Real sum and
printing it (in api.cpp within the same scope where Real sum and tenth are
used), add a check such as assert(sum == Real(1)) (or use an equivalent test
that reports failure) so the test fails automatically on regression; ensure you
include <cassert> or use the project’s existing test/assert mechanism consistent
with other tests.
- Around line 55-58: The variable test_tag is declared but never used; remove
the unused declaration of test_tag from the function (leaving test_suite,
reportTestCases, and nrOfFailedTestCases intact), or if you prefer to keep a
tag, use test_tag where test-suite reporting occurs (e.g., include it in any
log/report call that references test_suite or reportTestCases) so the symbol
test_tag is actually referenced; ensure there are no remaining references to the
removed symbol.
In `@static/float/dfloat/api/ranges.cpp`:
- Around line 18-21: The variable test_tag declared alongside test_suite,
reportTestCases and nrOfFailedTestCases is unused; remove the unused declaration
of test_tag from the top of ranges.cpp (or, if intended to be used, actually use
it in the test harness where other tags are referenced) so the compiler warning
is removed—look for the declaration "std::string test_tag" and either delete
that line or replace its use consistent with how test_suite/reportTestCases are
used in the surrounding test code.
In `@static/float/dfloat/standard/decimal64.cpp`:
- Around line 107-119: The test currently compares BID and DPD by converting
decimal64_bid and decimal64_dpd to double (variables decimal64_bid,
decimal64_dpd and comparisons of dbid/ddpd), which can hide precision/exponent
differences; change the test to unpack and compare the internal components
(significand/mantissa and exponent/scale) of decimal64_bid and decimal64_dpd
directly instead of casting to double—use the types' accessors or implement
helper functions to extract the significand and exponent for decimal64_bid and
decimal64_dpd, compare those fields and increment nrOfFailedTestCases and print
via reportTestCases on mismatch (keeping the same fail reporting flow).
- Around line 163-170: The preprocessor blocks for REGRESSION_LEVEL_2,
REGRESSION_LEVEL_3, and REGRESSION_LEVEL_4 in decimal64.cpp are empty; either
remove these empty `#if` ... `#endif` sections or add a short TODO comment inside
each (e.g., "// TODO: implement regression tests for LEVEL_X") so the intent is
clear and the placeholders aren't silent; locate the three blocks guarded by
REGRESSION_LEVEL_2, REGRESSION_LEVEL_3, and REGRESSION_LEVEL_4 and update them
accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
include/sw/universal/number/dfloat/attributes.hppinclude/sw/universal/number/dfloat/dfloat.hppinclude/sw/universal/number/dfloat/dfloat_impl.hppinclude/sw/universal/number/hfloat/hfloat_impl.hppplayground/CMakeLists.txtplayground/regression_test_skeleton.cppstatic/float/dfloat/api/api.cppstatic/float/dfloat/api/ranges.cppstatic/float/dfloat/standard/decimal128.cppstatic/float/dfloat/standard/decimal64.cpp
💤 Files with no reviewable changes (2)
- include/sw/universal/number/dfloat/dfloat.hpp
- static/float/dfloat/standard/decimal128.cpp
Pull Request Test Coverage Report for Build 22510678448Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
__uint128_twithblockbinary<>: Refactors dfloat encoding storage (bt _block[]→blockbinary<nbits>) and significand arithmetic (uint64_t/__uint128_t→blockbinary<4*ndigits+8>), eliminating all__SIZEOF_INT128__guards. Makesdecimal128fully portable to MSVC and extensible to arbitrary precision beyond 38 digits.assign()and the string constructor now parse fixed ("999.9999"), scientific ("-123.456e-78"), integer, fractional, and special value formats with exact decimal semantics — no binary conversion.str()andoperator<<formatting: Adds scientific/fixed/automatic format modes respecting iostream flags. Defaults to showing allndigitssignificant digits (exact decimal digits shouldn't be truncated by the iostream default precision of 6). Fixesdynamic_range()which was casting todoubleand overflowing toinffor decimal64/128 extreme values.Test plan
__SIZEOF_INT128__references remain in dfloat source/test directoriesdecimal128compiles and passes without platform guardsnumeric_limits::max()/min()print correctly in scientific notationdynamic_range()shows correct finite values for decimal64/128🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests