[PATCH] Support/APFloat, fix assert on convertFromString(toString(infty))
Johan Engelen
jbc.engelen at swissonline.ch
Mon Mar 23 13:00:35 PDT 2015
On 22-3-2015 17:39, Duncan P. N. Exon Smith wrote:
>> 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`.
I was mimicking surrounding code. Large portions of the file violate the
80-column rule and use variables that start in lowercase. Nevertheless,
I've used clang-format for the attached patch (v3); and stopped using
variables ;)
> 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.
I did not add explicit checks for the generated string output, as I
figured that that is not what is causing (my) problem. Whatever string
is output by APFloat should be parsable by APFloat too; but I do not
know if there is a specific spec for what string to output, nor do I
want to start a discussion about whether to output "NaN" or "+NaN".
I did add explicit checks for a few special string parsing cases (those
strings tested can be output by APFloat).
I hope with this, the patch is OK to be committed, or that further
trivial changes to it can be done by the committer.
Thanks very much,
Johan
-------------- next part --------------
Index: lib/Support/APFloat.cpp
===================================================================
--- lib/Support/APFloat.cpp (revision 232993)
+++ lib/Support/APFloat.cpp (working copy)
@@ -2615,22 +2615,24 @@
bool
APFloat::convertFromStringSpecials(StringRef str) {
- if (str.equals("inf") || str.equals("INFINITY")) {
+ if (str.equals("inf") || str.equals("Inf") || str.equals("infinity") ||
+ str.equals("INFINITY")) {
makeInf(false);
return true;
}
- if (str.equals("-inf") || str.equals("-INFINITY")) {
+ if (str.equals("-inf") || str.equals("-Inf") || str.equals("-infinity") ||
+ str.equals("-INFINITY")) {
makeInf(true);
return true;
}
- if (str.equals("nan") || str.equals("NaN")) {
+ if (str.equals("nan") || str.equals("NaN") || str.equals("NAN")) {
makeNaN(false, false);
return true;
}
- if (str.equals("-nan") || str.equals("-NaN")) {
+ if (str.equals("-nan") || str.equals("-NaN") || str.equals("-NAN")) {
makeNaN(false, true);
return true;
}
@@ -3549,9 +3551,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 232993)
+++ unittests/ADT/APFloatTest.cpp (working copy)
@@ -844,6 +844,22 @@
EXPECT_TRUE(APFloat(APFloat::IEEEdouble, "-1e-99999").isNegZero());
EXPECT_EQ(2.71828, convertToDoubleFromString("2.71828"));
+
+ // Test the parsing of special numbers.
+ EXPECT_TRUE(APFloat::getInf(APFloat::IEEEdouble, false)
+ .bitwiseIsEqual(APFloat(APFloat::IEEEdouble, "inf")));
+ EXPECT_TRUE(APFloat::getInf(APFloat::IEEEdouble, true)
+ .bitwiseIsEqual(APFloat(APFloat::IEEEdouble, "-Inf")));
+ EXPECT_TRUE(APFloat::getInf(APFloat::IEEEdouble, false)
+ .bitwiseIsEqual(APFloat(APFloat::IEEEdouble, "INFINITY")));
+ EXPECT_TRUE(APFloat::getInf(APFloat::IEEEsingle, true)
+ .bitwiseIsEqual(APFloat(APFloat::IEEEsingle, "-infinity")));
+ EXPECT_TRUE(APFloat::getNaN(APFloat::IEEEsingle, false)
+ .bitwiseIsEqual(APFloat(APFloat::IEEEsingle, "NaN")));
+ EXPECT_TRUE(APFloat::getNaN(APFloat::IEEEquad, true)
+ .bitwiseIsEqual(APFloat(APFloat::IEEEquad, "-nan")));
+ EXPECT_TRUE(APFloat::getNaN(APFloat::IEEEdouble, true)
+ .bitwiseIsEqual(APFloat(APFloat::IEEEdouble, "-NAN")));
}
TEST(APFloatTest, fromHexadecimalString) {
@@ -949,6 +965,18 @@
ASSERT_EQ("1.7976931348623157E+308", convertToString(1.7976931348623157E+308, 0, 0));
}
+TEST(APFloatTest, StringConversionRoundtrip) {
+ #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
+}
+
TEST(APFloatTest, toInteger) {
bool isExact = false;
APSInt result(5, /*isUnsigned=*/true);
More information about the llvm-commits
mailing list