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

[Duncan P. N. Exon Smith]

> On 2015 Mar 21, at 16:17, Johan Engelen <jbc.engelen at swissonline.ch> wrote:
> 
> 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
> 
> 
> <APFloat_v2.patch>

Thanks for continuing to work on this.  Code changes look fine to me
still, but I have a few comments on the tests.

> 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

Comments should be complete sentences (they should end with periods).

Also, all these lines violate the 80-column rule.  I'd recommend
integrating `clang-format` [1] into your workflow, since it will handle
formatting for you.

[1]: http://clang.llvm.org/docs/ClangFormat.html

> +  double posInf = APFloat::getInf(APFloat::IEEEdouble, false).convertToDouble();
> +  EXPECT_EQ(posInf, convertToDoubleFromString(convertToString(posInf, 0, 3).c_str()));

Current style guidelines say variables should start with a capital
letter, so this would be `PosInf`.

However, this is all pretty mechanical.  I think you should use a macro:

    #define EXPECT_ROUNDTRIP(F)                                                    \
      EXPECT_TRUE((F).bitwiseIsEqual(APFloat(                                      \
          APFloat::IEEEdouble, convertToString((F).convertToDouble(), 0, 0))))
      EXPECT_ROUNDTRIP(APFloat::getInf(APFloat::IEEEdouble, false));
      EXPECT_ROUNDTRIP(APFloat::getInf(APFloat::IEEEdouble, true));
      EXPECT_ROUNDTRIP(APFloat::getNaN(APFloat::IEEEdouble, false));
      EXPECT_ROUNDTRIP(APFloat::getNaN(APFloat::IEEEdouble, true));
    #undef EXPECT_ROUNDTRIP

A few more points:

  - As a prep patch, change `convertToDouble()` to take a `StringRef`.
  - These would be better in a new test (maybe called
    `TEST(APFloatTest, StringRoundtrip)`?).
  - You should add explicit checks for `convertToString()` here (after
    moving your new tests elsewhere).
  - You should add "from-string" tests somewhere to explicitly check
    whether we can parse these special numbers.

> +  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) {
>