[PATCH] Support/APFloat, fix assert on convertFromString(toString(infty))

[Johan Engelen]

On 17-3-2015 18:46, Duncan P. N. Exon Smith wrote:
>> On 2015-Mar-16, at 15:54, Johan Engelen <jbc.engelen at swissonline.ch> wrote:
>>
>> Hello,
>>   Here is a patch for /lib/Support/APFloat.cpp that fixes an assert when the output of APFloat::toString() is fed back into APFloat::convertFromString().
>>
>> For infinity, toString() outputs a string with upper case "Inf", whereas convertFromString() only accepts lowercase "inf".
>>
>> The patch adds "Inf" and "-Inf" to the accepted strings in convertFromString(), and modifies the output from toString() to "Inf" instead of "+Inf" for positive infinity.
> Testcase?  I suggest something in unittests/ADT/APFloatTest.cpp.

Thanks for your email. Sorry for not providing tests, I'm new to LLVM 
development.
Attached is an improved patch with unittests added. I added an extra 
change that adds the sign of NaN to the string output.

kind regards,
   Johan


-------------- next part --------------
Index: lib/Support/APFloat.cpp
===================================================================
--- lib/Support/APFloat.cpp	(revision 232903)
+++ lib/Support/APFloat.cpp	(working copy)
@@ -2615,12 +2615,12 @@
 
 bool
 APFloat::convertFromStringSpecials(StringRef str) {
-  if (str.equals("inf") || str.equals("INFINITY")) {
+  if (str.equals("inf") || str.equals("Inf") || str.equals("INFINITY")) {
     makeInf(false);
     return true;
   }
 
-  if (str.equals("-inf") || str.equals("-INFINITY")) {
+  if (str.equals("-inf") || str.equals("-Inf") || str.equals("-INFINITY")) {
     makeInf(true);
     return true;
   }
@@ -3549,9 +3549,13 @@
     if (isNegative())
       return append(Str, "-Inf");
     else
-      return append(Str, "+Inf");
+      return append(Str, "Inf");
 
-  case fcNaN: return append(Str, "NaN");
+  case fcNaN:
+    if (isNegative())
+      return append(Str, "-NaN");
+    else
+      return append(Str, "NaN");
 
   case fcZero:
     if (isNegative())
Index: unittests/ADT/APFloatTest.cpp
===================================================================
--- unittests/ADT/APFloatTest.cpp	(revision 232903)
+++ unittests/ADT/APFloatTest.cpp	(working copy)
@@ -947,6 +947,19 @@
   ASSERT_EQ("873.18340000000001", convertToString(873.1834, 0, 1));
   ASSERT_EQ("8.7318340000000001E+2", convertToString(873.1834, 0, 0));
   ASSERT_EQ("1.7976931348623157E+308", convertToString(1.7976931348623157E+308, 0, 0));
+
+  // Check that Infinity and NaN convert to a string that is interpreted as the same APFloat
+  double posInf = APFloat::getInf(APFloat::IEEEdouble, false).convertToDouble();
+  EXPECT_EQ(posInf, convertToDoubleFromString(convertToString(posInf, 0, 3).c_str()));
+  double negInf = APFloat::getInf(APFloat::IEEEdouble, true).convertToDouble();
+  EXPECT_EQ(negInf, convertToDoubleFromString(convertToString(negInf, 5, 2).c_str()));
+  // NaN does not compare with itself (assert(posNaN != posNaN)), so do a bitwise compare instead
+  APFloat posNaN = APFloat::getNaN(APFloat::IEEEdouble, false);
+  EXPECT_TRUE(posNaN.bitwiseIsEqual( 
+              APFloat(APFloat::IEEEdouble, convertToString(posNaN.convertToDouble(), 5, 0).c_str()) ));
+  APFloat negNaN = APFloat::getNaN(APFloat::IEEEdouble, true);
+  EXPECT_TRUE(negNaN.bitwiseIsEqual( 
+              APFloat(APFloat::IEEEdouble, convertToString(negNaN.convertToDouble(), 3, 1).c_str()) ));
 }
 
 TEST(APFloatTest, toInteger) {