Browse Source

String: Fix default decimals truncation in num and num_real

Fixes undefined behavior, and fixes the logic for negative powers of ten.
Fixes #51764.

Adds tests to validate the changes and prevent regressions.
Adds docs for `String.num`.
Rémi Verschelde 4 years ago
parent
commit
066dbc2f0c
3 changed files with 59 additions and 7 deletions
  1. 20 7
      core/string/ustring.cpp
  2. 15 0
      doc/classes/String.xml
  3. 24 0
      tests/test_string.h

+ 20 - 7
core/string/ustring.cpp

@@ -1396,7 +1396,13 @@ String String::num(double p_num, int p_decimals) {
 #ifndef NO_USE_STDLIB
 
 	if (p_decimals < 0) {
-		p_decimals = 14 - (int)floor(log10(p_num));
+		p_decimals = 14;
+		const double abs_num = ABS(p_num);
+		if (abs_num > 10) {
+			// We want to align the digits to the above sane default, so we only
+			// need to subtract log10 for numbers with a positive power of ten.
+			p_decimals -= (int)floor(log10(abs_num));
+		}
 	}
 	if (p_decimals > MAX_DECIMALS) {
 		p_decimals = MAX_DECIMALS;
@@ -1625,24 +1631,31 @@ String String::num_real(double p_num, bool p_trailing) {
 
 	String s;
 	String sd;
-	/* integer part */
+
+	// Integer part.
 
 	bool neg = p_num < 0;
 	p_num = ABS(p_num);
 	int intn = (int)p_num;
 
-	/* decimal part */
+	// Decimal part.
 
-	if ((int)p_num != p_num) {
-		double dec = p_num - (double)((int)p_num);
+	if (intn != p_num) {
+		double dec = p_num - (double)(intn);
 
 		int digit = 0;
 
 #if REAL_T_IS_DOUBLE
-		int decimals = 14 - (int)floor(log10(p_num));
+		int decimals = 14;
 #else
-		int decimals = 6 - (int)floor(log10(p_num));
+		int decimals = 6;
 #endif
+		// We want to align the digits to the above sane default, so we only
+		// need to subtract log10 for numbers with a positive power of ten.
+		if (p_num > 10) {
+			decimals -= (int)floor(log10(p_num));
+		}
+
 		if (decimals > MAX_DECIMALS) {
 			decimals = MAX_DECIMALS;
 		}

+ 15 - 0
doc/classes/String.xml

@@ -410,6 +410,21 @@
 			<argument index="0" name="number" type="float" />
 			<argument index="1" name="decimals" type="int" default="-1" />
 			<description>
+				Converts a [float] to a string representation of a decimal number.
+				The number of decimal places can be specified with [code]decimals[/code]. If [code]decimals[/code] is [code]-1[/code] (default), decimal places will be automatically adjusted so that the string representation has 14 significant digits (counting both digits to the left and the right of the decimal point).
+				Trailing zeros are not included in the string. The last digit will be rounded and not truncated.
+				Some examples:
+				[codeblock]
+				String.num(3.141593)     # "3.141593"
+				String.num(3.141593, 3)  # "3.142"
+				String.num(3.14159300)   # "3.141593", no trailing zeros.
+				# Last digit will be rounded up here, which reduces total digit count since
+				# trailing zeros are removed:
+				String.num(42.129999, 5) # "42.13"
+				# If `decimals` is not specified, the total amount of significant digits is 14:
+				String.num(-0.0000012345432123454321)     # "-0.00000123454321"
+				String.num(-10000.0000012345432123454321) # "-10000.0000012345"
+				[/codeblock]
 			</description>
 		</method>
 		<method name="num_scientific" qualifiers="static">

+ 24 - 0
tests/test_string.h

@@ -350,6 +350,9 @@ TEST_CASE("[String] Insertion") {
 }
 
 TEST_CASE("[String] Number to string") {
+	CHECK(String::num(0) == "0");
+	CHECK(String::num(0.0) == "0"); // No trailing zeros.
+	CHECK(String::num(-0.0) == "-0"); // Includes sign even for zero.
 	CHECK(String::num(3.141593) == "3.141593");
 	CHECK(String::num(3.141593, 3) == "3.142");
 	CHECK(String::num_real(3.141593) == "3.141593");
@@ -357,6 +360,27 @@ TEST_CASE("[String] Number to string") {
 	CHECK(String::num_int64(3141593) == "3141593");
 	CHECK(String::num_int64(0xA141593, 16) == "a141593");
 	CHECK(String::num_int64(0xA141593, 16, true) == "A141593");
+	CHECK(String::num(42.100023, 4) == "42.1"); // No trailing zeros.
+
+	// Checks doubles with many decimal places.
+	CHECK(String::num(0.0000012345432123454321, -1) == "0.00000123454321"); // -1 uses 14 as sane default.
+	CHECK(String::num(0.0000012345432123454321) == "0.00000123454321"); // -1 is the default value.
+	CHECK(String::num(-0.0000012345432123454321) == "-0.00000123454321");
+	CHECK(String::num(-10000.0000012345432123454321) == "-10000.0000012345");
+	CHECK(String::num(0.0000000000012345432123454321) == "0.00000000000123");
+	CHECK(String::num(0.0000000000012345432123454321, 3) == "0");
+
+	// Note: When relevant (remainder > 0.5), the last digit gets rounded up,
+	// which can also lead to not include a trailing zero, e.g. "...89" -> "...9".
+	CHECK(String::num(0.0000056789876567898765) == "0.00000567898766"); // Should round last digit.
+	CHECK(String::num(10000.000005678999999999) == "10000.000005679"); // We cut at ...789|99 which is rounded to ...79, so only 13 decimals.
+	CHECK(String::num(42.12999999, 6) == "42.13"); // Also happens with lower decimals count.
+
+	// 32 is MAX_DECIMALS. We can't reliably store that many so we can't compare against a string,
+	// but we can check that the string length is 34 (32 + 2 for "0.").
+	CHECK(String::num(0.00000123456789987654321123456789987654321, 32).length() == 34);
+	CHECK(String::num(0.00000123456789987654321123456789987654321, 42).length() == 34); // Should enforce MAX_DECIMALS.
+	CHECK(String::num(10000.00000123456789987654321123456789987654321, 42).length() == 38); // 32 decimals + "10000.".
 }
 
 TEST_CASE("[String] String to integer") {