Skip to content

Relax a few unittests for double-precision reals #5346

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dlang-bot merged 1 commit into from
May 24, 2017
Merged

Relax a few unittests for double-precision reals #5346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions std/format.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,17 +685,17 @@ uint formattedRead(R, Char, S...)(ref R r, const(Char)[] fmt, auto ref S args)
assert(a == "hello" && b == 124 && c == 34.5);

// mix pointers and auto-ref
s = "world!200:42.2";
s = "world!200:42.25";
formattedRead(s, "%s!%s:%s", a, &b, &c);
assert(a == "world" && b == 200 && c == 42.2);
assert(a == "world" && b == 200 && c == 42.25);

s = "world1!201:42.3";
s = "world1!201:42.5";
formattedRead(s, "%s!%s:%s", &a, &b, c);
assert(a == "world1" && b == 201 && c == 42.3);
assert(a == "world1" && b == 201 && c == 42.5);

s = "world2!202:42.4";
s = "world2!202:42.75";
formattedRead(s, "%s!%s:%s", a, b, &c);
assert(a == "world2" && b == 202 && c == 42.4);
assert(a == "world2" && b == 202 && c == 42.75);
}

// for backwards compatibility
Expand Down
9 changes: 7 additions & 2 deletions std/json.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1706,12 +1706,14 @@ pure nothrow @safe unittest // issue 15884

@safe unittest // issue 15885
{
enum bool realInDoublePrecision = real.mant_dig == double.mant_dig;

static bool test(const double num0)
{
import std.math : feqrel;
const json0 = JSONValue(num0);
const num1 = to!double(toJSON(json0));
version(Win32)
static if (realInDoublePrecision)
Copy link
Contributor

@tsbockman tsbockman Apr 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By doing this you may be relaxing the test on platforms where it currently works, inviting regressions.

Copy link
Contributor Author

@kinke kinke Apr 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since when does DMD cover 64-bit reals? Most new Phobos releases come with a few new tests that fail exactly because they're not tested with 64-bit reals, so I keep on upstreaming relaxed unittests for LDC which actually supports and tests targets with double-precision reals.

If you consider the potential being-off-by-1-ulp as bug (for std.format and/or std.json?), the Win32 exception shouldn't have been here already in the first place. I'm just extending that exception to its true scope here.

Copy link
Contributor

@tsbockman tsbockman Apr 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a list online somewhere of which platforms have is(double == real)?

Copy link
Contributor Author

@kinke kinke Apr 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Afaik, we use it for the 32/64-bit MSVC targets (i.e., on Windows), PowerPC and 32-bit ARM and possibly others, although real is still a distinct type (mangling).

Copy link
Contributor

@joakim-noah joakim-noah Apr 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I run these tests on Android/ARM, which is one of the platforms where real is 64-bit, and can confirm that this one only fails because of variability in the last digit, and that this relaxation patch gets it to pass again.

Copy link
Member

@ibuclaw ibuclaw Apr 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a list online somewhere of which platforms have is(double == real)?

It would be easier to list those that don't. x86 (80-bit) and PPC (64-bit pair) for sure, I think IA64 (128-bit) is in that list too. Everyone else is a platform where real == double.

Copy link
Contributor Author

@kinke kinke Apr 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AArch64 has 128-bit quadruple reals. The PowerPC doubledouble is implemented in software AFAIK, that's why we use double-precision reals for PowerPC.

return feqrel(num1, num0) >= (double.mant_dig - 1);
else
return num1 == num0;
Expand All @@ -1725,8 +1727,11 @@ pure nothrow @safe unittest // issue 15884
assert(test(30738.22));

assert(test(1 + double.epsilon));
assert(test(-double.max));
assert(test(double.min_normal));
static if (realInDoublePrecision)
assert(test(-double.max / 2));
else
assert(test(-double.max));
Copy link
Contributor

@tsbockman tsbockman Apr 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the result of this test when it fails? What consequences could the failure have for the API user?

Copy link
Contributor

@tsbockman tsbockman Apr 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answering my own question (from Relax std.json unittest some more): "test(-double.max) results in an overflow and thrown exception in std.conv.parse!(double, string)."

I guess that's not too bad; at least it doesn't just silently corrupt data.

Copy link
Contributor Author

@kinke kinke Apr 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may have been a more severe issue (it's been a while since I relaxed them for LDC), I need to recheck.
Edit: Thanks for checking, didn't see your 2nd post before.


const minSub = double.min_normal * double.epsilon;
assert(test(minSub));
Expand Down