Skip to content

Commit 9c389f8

Browse files
committed
fix(String.Format, Int32.ToString): apply custom numeric formats and stop widening a float
Three defects in .NET numeric formatting, all found while sweeping cna-samples for sample-local reimplementations of runtime API. Converting an XNA sample's snprintf("%02d:%02d") to the faithful String.Format("{0:00}:{1:00}") would have been a regression rather than a fix, which is how these surfaced. 1. String::Format silently DROPPED a custom numeric format string. It carried a second numeric formatter of its own, and that one knew only X/x/D/d for integers and F/G/E for doubles, so every custom format fell through to a plain decimal: Format("{0:00}:{1:00}", 3, 7) returned "3:7" where .NET returns "03:07", and Format("{0:0.00}", 59.4) returned "59.4" instead of "59.40". A wrong answer with no diagnostic. fmtInt and fmtDouble now defer to the type's own ToString for that case, which already implements the grammar. 2. Int32::ToString had no custom-numeric path at all, though Single and Double both did, so int.ToString("00") threw FormatException on a format .NET formats fine. It now takes the same branch they do. 3. String::Format(fmt, float) forwarded to the double overload, so the argument was formatted through Double's round-trip digits: "{0}" on 59.4f produced "59.400001525878906" where .NET prints "59.4", because .NET formats the argument with its own Single.ToString(). FormatArg grew a Float kind. The gate is deliberately narrower than "not a standard numeric format", which is also true of a MALFORMED standard specifier -- "DX", "D-3", "X99999999999999999999" -- whose behaviour tickets #1847/#1849 pinned and which must not change. isCustomNumericPlaceholderFormat asks the narrower question the callers mean: does the format begin with something other than a specifier letter and contain a digit placeholder? A first attempt that delegated the whole specifier hung StringFormatBoundaryTests.SpecifierBoundIsTheReferenceBound, because "{0:D1000000000}" reached Int32::ToString's own padding loop, which has no such bound -- caught by the suite, not by inspection. Four tests, each confirmed to fail with its own fix reverted. Full suite: 17875/17875.
1 parent 170e4ae commit 9c389f8

5 files changed

Lines changed: 142 additions & 2 deletions

File tree

modules/core/include/System/Int32.hpp

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313
#include <stdexcept>
1414
#include <string>
1515
#include <utility>
16+
#include "System/NotSupportedException.hpp"
17+
#include "System/detail/FloatTextFormat.hpp"
1618
#include "SharpRuntime/SharpRuntimeHelper.hpp"
1719
#include "System/ArgumentException.hpp"
1820
#include "System/ArgumentOutOfRangeException.hpp"
@@ -199,6 +201,31 @@ class Int32 {
199201
*/
200202
static std::string ToString(SharpRuntime::intcs value, const std::string& format) {
201203
if (format.empty()) return std::to_string(value);
204+
// .NET reads a format as standard only when it is one letter plus an optional
205+
// precision. Anything else -- "00", "0.0", "#,##0" -- is a CUSTOM numeric format
206+
// string, a separate grammar the standard specifiers below cannot express, and
207+
// Int32 supports it exactly as Single and Double already do. Without this branch
208+
// `int.ToString("00")` -- and therefore `String.Format("{0:00}", n)`, which several
209+
// XNA samples use to print a two-digit clock -- raised FormatException on a format
210+
// .NET formats fine.
211+
if (System::detail::isCustomNumericPlaceholderFormat(format)) {
212+
const System::detail::CustomNumericFormat shape =
213+
System::detail::parseCustomNumericFormat(format, [] {
214+
throw System::NotSupportedException(
215+
"This custom numeric format string uses a construct that is not "
216+
"implemented: section separators, percent/permille scaling, custom "
217+
"exponent forms and escaping are not supported.");
218+
});
219+
bool negative = false;
220+
std::string integerDigits;
221+
std::string fractionDigits;
222+
System::detail::splitDecimalText(std::to_string(value), negative, integerDigits,
223+
fractionDigits);
224+
System::detail::roundDecimalDigits(integerDigits, fractionDigits,
225+
shape.hasDecimalPoint ? shape.maximumDecimals : 0);
226+
return System::detail::emitCustomNumeric(negative, integerDigits, fractionDigits,
227+
format, shape);
228+
}
202229
char type = format[0];
203230
int width = 0;
204231
if (format.size() > 1) {

modules/core/include/System/detail/FloatTextFormat.hpp

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,27 @@ template <class T>
133133
return true;
134134
}
135135

136+
/**
137+
* @brief True when @p format is unambiguously a CUSTOM numeric format string.
138+
*
139+
* Stricter than `!isStandardNumericFormat(format)` on purpose. That test is also false for a
140+
* *malformed standard* specifier -- "DX", "D-3", "X99999999999999999999" -- and those have their
141+
* own long-standing, tested behaviour in the integer wrappers and in String::Format (a
142+
* FormatException, or the tail ignored) that must not change. This asks the narrower question the
143+
* callers actually mean: does the format begin with something other than a specifier letter and
144+
* contain a digit placeholder? "00", "0.00" and "#,##0" do; every malformed standard specifier
145+
* begins with its letter and is left alone.
146+
*
147+
* @param format The format string to classify.
148+
* @return True when @p format should be handled by the custom numeric grammar.
149+
*/
150+
[[nodiscard]] inline bool isCustomNumericPlaceholderFormat(const std::string& format) {
151+
if (format.empty()) return false;
152+
const unsigned char first = static_cast<unsigned char>(format[0]);
153+
if ((first >= 'A' && first <= 'Z') || (first >= 'a' && first <= 'z')) return false;
154+
return format.find_first_of("0#") != std::string::npos;
155+
}
156+
136157
/**
137158
* @brief The subset of .NET's custom numeric format grammar this build implements.
138159
*

modules/core/src/System/String.cpp

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,10 @@
33
// Portions based on .NET runtime API (MIT License, Copyright .NET Foundation and Contributors)
44
#include "System/String.hpp"
55
#include "System/detail/CompositeFormat.hpp"
6+
#include "System/detail/FloatTextFormat.hpp"
7+
#include "System/Double.hpp"
8+
#include "System/Int32.hpp"
9+
#include "System/Single.hpp"
610
#include "System/ArgumentOutOfRangeException.hpp"
711
#include "System/FormatException.hpp"
812
#include "System/OutOfMemoryException.hpp"
@@ -150,16 +154,22 @@ namespace System
150154
// format item. `text` points at the caller's own std::string parameter, which outlives
151155
// the call; nothing here owns or copies it.
152156
struct FormatArg {
153-
enum class Kind { Int, Long, Double, Text };
157+
enum class Kind { Int, Long, Float, Double, Text };
154158
Kind kind = Kind::Int;
155159
SharpRuntime::intcs i = 0;
156160
SharpRuntime::longcs l = 0;
161+
float f = 0.0f;
157162
double d = 0.0;
158163
const std::string* text = nullptr;
159164
};
160165

161166
FormatArg argOf(SharpRuntime::intcs v) { FormatArg a; a.kind = FormatArg::Kind::Int; a.i = v; return a; }
162167
FormatArg argOf(SharpRuntime::longcs v) { FormatArg a; a.kind = FormatArg::Kind::Long; a.l = v; return a; }
168+
// A float is NOT widened to double. Single and Double round-trip through a different
169+
// number of digits, so Format("{0}", 59.4f) came out as "59.400001525878906" -- the
170+
// double text of the widened float -- where .NET prints "59.4", because .NET formats the
171+
// argument with its OWN Single.ToString().
172+
FormatArg argOf(float v) { FormatArg a; a.kind = FormatArg::Kind::Float; a.f = v; return a; }
163173
FormatArg argOf(double v) { FormatArg a; a.kind = FormatArg::Kind::Double; a.d = v; return a; }
164174
FormatArg argOf(const std::string& v) { FormatArg a; a.kind = FormatArg::Kind::Text; a.text = &v; return a; }
165175

@@ -211,6 +221,19 @@ namespace System
211221
// Format integer with .NET-style specifier (X/x=hex, D=decimal padded, else plain).
212222
std::string fmtInt(SharpRuntime::intcs value, std::string_view spec) {
213223
if (spec.empty()) return std::to_string(value);
224+
// A format that is not one letter plus an optional precision is a CUSTOM numeric
225+
// format string -- "00", "0.00", "#,##0" -- a separate .NET grammar the standard
226+
// specifiers below cannot express. This function knew only X/x/D/d, so every custom
227+
// format fell through to a plain decimal and was silently DROPPED:
228+
// Format("{0:00}:{1:00}", 3, 7) returned "3:7" where .NET returns "03:07". The
229+
// grammar is already implemented once, in the type's own ToString, so this defers to
230+
// it rather than growing a second copy. Standard specifiers keep taking the path
231+
// below unchanged, which is what preserves the specifier-tail hardening of tickets
232+
// #1847/#1849 -- ToString's own tail parsing is stricter in ways Format's callers
233+
// are already tested against ("{0:DX}" must yield "42", not throw).
234+
if (System::detail::isCustomNumericPlaceholderFormat(std::string(spec))) {
235+
return System::Int32::ToString(value, std::string(spec));
236+
}
214237
const char sc = spec[0];
215238
const SpecNumber num = parseSpecNumber(spec);
216239
if (num.kind == SpecNumberKind::TooLarge) throwBadFormatSpecifier();
@@ -270,6 +293,11 @@ namespace System
270293
auto [ptr, ec] = std::to_chars(buf.data(), buf.data() + buf.size(), value);
271294
return ec == std::errc{} ? std::string(buf.data(), ptr) : std::to_string(value);
272295
}
296+
// Same custom-format gap as fmtInt above, in the floating-point half:
297+
// Format("{0:0.00}", 59.4) returned "59.4" instead of "59.40".
298+
if (System::detail::isCustomNumericPlaceholderFormat(std::string(spec))) {
299+
return System::Double::ToString(value, std::string(spec));
300+
}
273301
const char sc = spec[0];
274302
const SpecNumber num = parseSpecNumber(spec);
275303
if (num.kind == SpecNumberKind::TooLarge) throwBadFormatSpecifier();
@@ -298,9 +326,20 @@ namespace System
298326
}
299327
}
300328

329+
// Format a float as .NET does: through Single's own ToString, never through Double's.
330+
// The specifier tail is validated here first, so an oversized one is still the
331+
// FormatException ticket #1849 pinned rather than a 10^9-digit precision request.
332+
std::string fmtFloat(float value, std::string_view spec) {
333+
if (spec.empty()) return System::Single::ToString(value);
334+
const SpecNumber num = parseSpecNumber(spec);
335+
if (num.kind == SpecNumberKind::TooLarge) throwBadFormatSpecifier();
336+
return System::Single::ToString(value, std::string(spec));
337+
}
338+
301339
std::string renderArg(const FormatArg& arg, std::string_view spec) {
302340
switch (arg.kind) {
303341
case FormatArg::Kind::Int: return fmtInt(arg.i, spec);
342+
case FormatArg::Kind::Float: return fmtFloat(arg.f, spec);
304343
case FormatArg::Kind::Double: return fmtDouble(arg.d, spec);
305344
case FormatArg::Kind::Long: return std::to_string(arg.l);
306345
case FormatArg::Kind::Text: return *arg.text;
@@ -838,7 +877,8 @@ namespace System
838877

839878
std::string String::Format(const std::string& format, float arg0)
840879
{
841-
return Format(format, static_cast<double>(arg0));
880+
const FormatArg args[] = {argOf(arg0)};
881+
return formatCore(format, args, 1);
842882
}
843883

844884
std::string String::Format(const std::string& format, SharpRuntime::longcs arg0, SharpRuntime::longcs arg1)

modules/core/tests/System/Int32NewTests.cpp

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Copyright (c) Robert Vokac and contributors
33
// Portions based on .NET runtime API (MIT License, Copyright .NET Foundation and Contributors)
44
#include <gtest/gtest.h>
5+
#include "System/FormatException.hpp"
56
#include "System/Int32.hpp"
67

78
using System::Int32;
@@ -39,3 +40,23 @@ TEST(Int32NewTests, MinMagnitude_MinValueAlwaysLoses) {
3940
EXPECT_EQ(Int32::MinMagnitude(Int32::MinValue, Int32::MinValue), Int32::MinValue);
4041
EXPECT_EQ(Int32::MinMagnitude(Int32::MinValue, Int32::MaxValue), Int32::MaxValue);
4142
}
43+
44+
TEST(Int32NewTests, ToString_CustomNumericFormat) {
45+
// Int32 lacked the custom-numeric path Single and Double already had, so int.ToString("00")
46+
// -- and therefore String.Format("{0:00}", n) -- raised FormatException on a format .NET
47+
// formats fine. Found by cna-samples SAMPLE-046, where two ported XNA samples print a clock
48+
// with String.Format("{0:00}:{1:00}", minutes, seconds).
49+
EXPECT_EQ(Int32::ToString(3, "00"), "03");
50+
EXPECT_EQ(Int32::ToString(3, "000"), "003");
51+
EXPECT_EQ(Int32::ToString(-3, "00"), "-03");
52+
EXPECT_EQ(Int32::ToString(1234, "00"), "1234");
53+
EXPECT_EQ(Int32::ToString(3, "0.0"), "3.0");
54+
}
55+
56+
TEST(Int32NewTests, ToString_MalformedStandardSpecifierIsUnaffectedByTheCustomPath) {
57+
// The custom path must not swallow a malformed STANDARD specifier: those still throw, which
58+
// is what ticket #1847 pinned. The gate asks whether the format begins with something other
59+
// than a specifier letter, not merely whether it fails to be a standard format.
60+
EXPECT_THROW(Int32::ToString(5, "Xz"), System::FormatException);
61+
EXPECT_THROW(Int32::ToString(5, "Q"), System::FormatException);
62+
}

modules/core/tests/System/StringTests.cpp

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1120,6 +1120,37 @@ TEST(StringFormatBoundaryTests, SpecifierBoundIsTheReferenceBound) {
11201120
EXPECT_THROW(String::Format("{0:D2147483648}", 7), System::FormatException);
11211121
}
11221122

1123+
TEST(StringFormatBoundaryTests, CustomNumericFormatIsAppliedNotDropped) {
1124+
// Format carried a second, partial numeric formatter that knew only X/x/D/d for integers
1125+
// and F/G/E for doubles, so every CUSTOM numeric format string fell through to a plain
1126+
// decimal and was silently DROPPED -- the wrong answer with no diagnostic. Both of these
1127+
// returned "3:7" and "59.4" before; .NET returns what is asserted here. Found by
1128+
// cna-samples SAMPLE-046: MarbleMaze and HoneycombRush both print a clock with
1129+
// String.Format("{0:00}:{1:00}", minutes, seconds).
1130+
EXPECT_EQ(String::Format("{0:00}:{1:00}", 3, 7), "03:07");
1131+
EXPECT_EQ(String::Format("{0:0.00}", 59.4), "59.40");
1132+
EXPECT_EQ(String::Format("{0:0.00}", 59.4f), "59.40");
1133+
EXPECT_EQ(String::Format("{0:000}", 7), "007");
1134+
}
1135+
1136+
TEST(StringFormatBoundaryTests, AFloatIsFormattedAsSingleNotAsWidenedDouble) {
1137+
// The float overload forwarded to the double one, so the argument was formatted through
1138+
// Double's round-trip digits: "{0}" on 59.4f produced "59.400001525878906". .NET formats
1139+
// the argument with its OWN Single.ToString().
1140+
EXPECT_EQ(String::Format("{0}", 59.4f), "59.4");
1141+
EXPECT_EQ(String::Format("{0}", 0.1f), "0.1");
1142+
EXPECT_EQ(String::Format("{0}", 3.1415927f), "3.1415927");
1143+
}
1144+
1145+
TEST(StringFormatBoundaryTests, CustomFormatDoesNotCaptureAMalformedStandardSpecifier) {
1146+
// The gate is deliberately narrower than "not a standard format": a malformed standard
1147+
// specifier is also not a standard format, and those have their own tested behaviour that
1148+
// must not change. A custom format begins with something other than a specifier letter.
1149+
EXPECT_EQ(String::Format("{0:DX}", 42), "42");
1150+
EXPECT_EQ(String::Format("{0:D-3}", 7), "007");
1151+
EXPECT_THROW(String::Format("{0:D1000000000}", 7), System::FormatException);
1152+
}
1153+
11231154
TEST(StringFormatBoundaryTests, NonNumericSpecifierTailDoesNotThrowStdException) {
11241155
// Was std::invalid_argument escaping from std::stoi. This port does not implement
11251156
// custom numeric format strings, so the value is emitted with no specifier applied.

0 commit comments

Comments
 (0)