fix(ereal): implement flag-honoring operator<<() for decimal output#538
Open
Ravenwater wants to merge 2 commits intomainfrom
Open
fix(ereal): implement flag-honoring operator<<() for decimal output#538Ravenwater wants to merge 2 commits intomainfrom
Ravenwater wants to merge 2 commits intomainfrom
Conversation
Replace the TBD stub in ereal's operator<<() with a complete ostream implementation that honors all standard formatting flags (fixed, scientific, precision, width, fill, showpos, uppercase, left/right alignment). Port the decimal conversion algorithm from dd_impl.hpp, adapted for ereal's multi-component expansion arithmetic with a guard against empty limb vectors during digit extraction. Add ostream_formatting.cpp regression test covering all formatting modes, special values, alignment, and extended precision output. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds a public string-formatting API ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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: 3
🧹 Nitpick comments (1)
elastic/ereal/api/ostream_formatting.cpp (1)
90-128: Add focused regressions for sub-unit fixed values andstd::internalwith sign.Current cases miss two high-value edges: fixed output for
(0,1)(e.g.,0.5) and internal padding withshowpos(sign should stay at column start).Suggested additions
int test_fixed_output() { int nrOfFailedTests = 0; + { + ereal<4> x(0.5); + std::string s = capture(x, 2, std::ios_base::fixed); + if (s != "0.50") { + std::cout << "FAIL: fixed output of 0.5 = '" << s << "', expected '0.50'\n"; + ++nrOfFailedTests; + } + } { ereal<4> x(3.14159); std::string s = capture(x, 3, std::ios_base::fixed); @@ int test_width_alignment() { int nrOfFailedTests = 0; @@ // Custom fill character { ereal<4> x(1.0); std::string s = capture(x, 2, std::ios_base::scientific, 20, '*'); if (s.find('*') == std::string::npos) { std::cout << "FAIL: custom fill output = '" << s << "', expected '*' fill\n"; ++nrOfFailedTests; } } + // Internal alignment with sign + { + ereal<4> x(1.0); + std::string s = capture(x, 2, + std::ios_base::scientific | std::ios_base::showpos | std::ios_base::internal, + 14, '_'); + if (s.empty() || s[0] != '+' || s[1] != '_') { + std::cout << "FAIL: internal showpos output = '" << s + << "', expected '+' followed by fill\n"; + ++nrOfFailedTests; + } + }Also applies to: 180-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elastic/ereal/api/ostream_formatting.cpp` around lines 90 - 128, Extend the test_fixed_output cases to cover the sub-unit fixed value and the internal-padding-with-sign scenario: add a case that constructs an ereal (e.g., ereal<4> or ereal<8>) with value 0.5, call capture(value, precision, std::ios_base::fixed) and assert the output contains a decimal point, correct fractional digits and no exponent; add another case that uses std::internal with showpos and a width (e.g., std::setw(10) and std::showpos passed into capture or the stream) and assert the '+' sign remains at the start of the field (column 0) while padding is applied after the sign. Modify test_fixed_output (and mirror the same additions to the analogous test block referenced around lines 180-225) to include these assertions, referencing the existing capture function and ereal template types so the new checks integrate with the current test harness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/sw/universal/number/ereal/ereal_impl.hpp`:
- Around line 500-505: The internal-padding branch wrongly assumes only negative
values have a leading sign; update the logic in the block guarded by the
variable internal to inspect the resulting string s for a leading sign character
(e.g., '+' or '-') instead of relying solely on negative, and insert the fill
after that sign when present (otherwise insert at position 0). Modify the code
that currently uses negative and
s.insert(static_cast<std::string::size_type>(...), nrCharsToFill, fill) so it
computes an index like (s.size()>0 && (s[0]=='+'||s[0]=='-')) ? 1 : 0 and uses
that index for insertion; keep variables internal, negative, s, nrCharsToFill,
and fill unchanged otherwise.
- Around line 417-419: The decimal-scale calculation using
static_cast<int>(std::log10(std::fabs(_limb[0]))) miscomputes scales for values
in (0,1) because the cast truncates toward zero; update the calculation in the
ereal implementation so powerOfTenScale uses
std::floor(std::log10(std::fabs(_limb[0]))) (or std::floor on the log10 result)
before converting to int, then recompute integerDigits and nrDigits from that
floor result so fixed-format values in (0,1) get integerDigits = 0 and the "0."
prefix path is taken correctly (adjust references to powerOfTenScale,
integerDigits, nrDigits, _limb, fixed, precision).
- Around line 436-439: The early-return in the branch checking (fixed &&
(precision == 0) && (std::fabs(_limb[0]) < 1.0)) writes one digit into s and
returns before calling Fill(s), bypassing width/fill/alignment logic; change
this branch to append the '0' or '1' char to s but do not return — let execution
fall through so the later Fill(s) call (and any subsequent formatting logic)
runs; adjust any local flags or state if necessary to prevent additional digits
being appended later, but do not call return from inside that branch.
---
Nitpick comments:
In `@elastic/ereal/api/ostream_formatting.cpp`:
- Around line 90-128: Extend the test_fixed_output cases to cover the sub-unit
fixed value and the internal-padding-with-sign scenario: add a case that
constructs an ereal (e.g., ereal<4> or ereal<8>) with value 0.5, call
capture(value, precision, std::ios_base::fixed) and assert the output contains a
decimal point, correct fractional digits and no exponent; add another case that
uses std::internal with showpos and a width (e.g., std::setw(10) and
std::showpos passed into capture or the stream) and assert the '+' sign remains
at the start of the field (column 0) while padding is applied after the sign.
Modify test_fixed_output (and mirror the same additions to the analogous test
block referenced around lines 180-225) to include these assertions, referencing
the existing capture function and ereal template types so the new checks
integrate with the current test harness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 508d3e1f-7c1d-4580-b643-171e001e3a0c
📒 Files selected for processing (2)
elastic/ereal/api/ostream_formatting.cppinclude/sw/universal/number/ereal/ereal_impl.hpp
Pull Request Test Coverage Report for Build 22738809745Details
💛 - Coveralls |
- Use std::floor(std::log10(...)) instead of static_cast<int> truncation so values in (0,1) get correct powerOfTenScale (e.g., 0.5 → "0.50") - Remove early return in fixed/precision-0/sub-unit branch so width/fill/alignment logic is not bypassed - Fix std::internal padding to check actual leading sign character instead of only the negative flag, so showpos '+' is handled correctly - Add test cases for sub-unit fixed output and internal+showpos padding Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@elastic/ereal/api/ostream_formatting.cpp`:
- Around line 56-57: The test currently indexes the string variable s with s[0]
and s.back() (e.g., in the "-42.5" default-output check) without verifying it's
non-empty; change these checks to first assert !s.empty() (or s.empty() and fail
with a clear message) and then use s.front() / s.back() for the character
comparisons, updating the failure messages to report an empty output when
appropriate; apply the same guard-and-check pattern to all other checks that use
s[0] or s.back() (the occurrences flagged in this file).
In `@include/sw/universal/number/ereal/ereal_impl.hpp`:
- Around line 638-641: to_digits() currently returns early on error without
normalizing the output, leaving exponent and the digit buffer in a partially
populated state; update both early-return paths (the one around the check using
_ten/_one and the similar block around lines 674-677) to set a deterministic
safe fallback (e.g., set exponent to 0 and normalize the digit buffer to
represent zero: digits[0]=0 and ndigits=1 or equivalent members used by this
implementation) before returning so callers receive a well-defined, normalized
representation on failure.
- Around line 495-498: Guard against negative std::streamsize width before
converting to size_t: check if width > 0 (or clamp with auto effectiveWidth =
std::max<std::streamsize>(0, width)) before computing strLength/ nrCharsToFill
and doing the static_cast to size_t; use effectiveWidth when calculating
nrCharsToFill and the comparison with strLength so negative widths behave like 0
and do not produce gigantic unsigned values (refer to the variables/identifiers
width, strLength, nrCharsToFill, and internal).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e330492-e213-41fa-961c-400b4d8c0e6e
📒 Files selected for processing (2)
elastic/ereal/api/ostream_formatting.cppinclude/sw/universal/number/ereal/ereal_impl.hpp
Comment on lines
+56
to
+57
| if (s[0] != '-') { | ||
| std::cout << "FAIL: default output of -42.5 = '" << s << "', expected leading '-'\n"; |
Contributor
There was a problem hiding this comment.
Avoid unchecked indexing in failure checks.
Several assertions index s[0] / s.back() without guarding emptiness. If formatting regresses to empty output, tests can crash instead of reporting a clean failure.
💡 Proposed fix pattern
- if (s[0] != '-') {
+ if (s.empty() || s[0] != '-') {
std::cout << "FAIL: default output of -42.5 = '" << s << "', expected leading '-'\n";
++nrOfFailedTests;
}
@@
- if (s.back() != ' ') {
+ if (s.empty() || s.back() != ' ') {
std::cout << "FAIL: left-aligned output = '" << s << "', expected trailing spaces\n";
++nrOfFailedTests;
}Also applies to: 147-148, 156-157, 203-204, 218-219, 240-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@elastic/ereal/api/ostream_formatting.cpp` around lines 56 - 57, The test
currently indexes the string variable s with s[0] and s.back() (e.g., in the
"-42.5" default-output check) without verifying it's non-empty; change these
checks to first assert !s.empty() (or s.empty() and fail with a clear message)
and then use s.front() / s.back() for the character comparisons, updating the
failure messages to report an empty output when appropriate; apply the same
guard-and-check pattern to all other checks that use s[0] or s.back() (the
occurrences flagged in this file).
Comment on lines
+495
to
+498
| size_t strLength = s.length(); | ||
| if (strLength < static_cast<size_t>(width)) { | ||
| size_t nrCharsToFill = (width - strLength); | ||
| if (internal) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
std::streamsize width C++ signed negative standard behavior formatted output
💡 Result:
std::ios_base::width() stores the minimum field width used by the next formatted output operation; std::setw(n) just sets that state. The parameter type is signed (std::streamsize), so passing a negative value is well-formed and not undefined by itself. [1]
For formatted output, the standard’s padding rule is effectively:
- padding happens only if
len(seq) < os.width() - otherwise, no padding is added. [2]
So if os.width() is negative, the condition len < width is false, and no padding occurs—i.e., a negative width behaves like “no minimum width” (same observable effect as width 0) for standard formatted output. [2]
(Implementations typically still “consume” the width for that operation by resetting it back to 0 afterward, as usual for formatted output.) [1]
Sources:
[1] cppreference: std::ios_base::width, std::setw notes https://en.cppreference.com/w/cpp/io/ios_base/width.html
[2] C++ draft wording: [ostream.formatted.reqmts] padding rule https://eel.is/c++draft/input.output
Guard negative stream width before size_t conversion.
width is std::streamsize (signed). Converting a negative width to size_t produces a large unsigned value, triggering unwanted padding insertion. The C++ standard specifies that negative width should behave like width 0 (no padding), but this code doesn't guard the conversion.
💡 Proposed fix
- size_t strLength = s.length();
- if (strLength < static_cast<size_t>(width)) {
+ size_t strLength = s.length();
+ if (width > 0 && strLength < static_cast<size_t>(width)) {
size_t nrCharsToFill = (width - strLength);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/sw/universal/number/ereal/ereal_impl.hpp` around lines 495 - 498,
Guard against negative std::streamsize width before converting to size_t: check
if width > 0 (or clamp with auto effectiveWidth = std::max<std::streamsize>(0,
width)) before computing strLength/ nrCharsToFill and doing the static_cast to
size_t; use effectiveWidth when calculating nrCharsToFill and the comparison
with strLength so negative widths behave like 0 and do not produce gigantic
unsigned values (refer to the variables/identifiers width, strLength,
nrCharsToFill, and internal).
Comment on lines
+638
to
+641
| if ((r >= _ten) || (r < _one)) { | ||
| std::cerr << "ereal::to_digits() failed to compute exponent\n"; | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Make to_digits() failure exits deterministic.
On the error returns, exponent/digit buffer are not normalized to a safe fallback. Callers can still consume partially populated digits and produce malformed output.
💡 Proposed fix
if ((r >= _ten) || (r < _one)) {
std::cerr << "ereal::to_digits() failed to compute exponent\n";
+ std::fill(s.begin(), s.end(), '0');
+ if (!s.empty()) s.back() = 0;
+ exponent = 0;
return;
}
@@
if (s[0] <= '0') {
std::cerr << "ereal::to_digits() non-positive leading digit\n";
+ std::fill(s.begin(), s.end(), '0');
+ if (!s.empty()) s.back() = 0;
+ exponent = 0;
return;
}Also applies to: 674-677
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/sw/universal/number/ereal/ereal_impl.hpp` around lines 638 - 641,
to_digits() currently returns early on error without normalizing the output,
leaving exponent and the digit buffer in a partially populated state; update
both early-return paths (the one around the check using _ten/_one and the
similar block around lines 674-677) to set a deterministic safe fallback (e.g.,
set exponent to 0 and normalize the digit buffer to represent zero: digits[0]=0
and ndigits=1 or equivalent members used by this implementation) before
returning so callers receive a well-defined, normalized representation on
failure.
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
TBDstub in ereal'soperator<<()with a complete ostream implementation that honors all standard formatting flags (std::fixed,std::scientific,std::setprecision,std::setw,std::showpos,std::uppercase,std::left/std::right/std::internal, fill character)dd_impl.hpp(to_string,to_digits,round_string,append_exponent), adapted for ereal's multi-component expansion arithmeticostream_formatting.cppregression test covering all formatting modes, special values, alignment, and extended precision outputTest plan
er_api_api— produces decimal values instead of "TBD"er_arith_addition,er_arith_subtraction,er_arith_multiplication,er_arith_division— no regressionser_api_ostream_formattingat all regression levels — PASS🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests