Skip to content
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.

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

fix Issue 22740 - float and double literals should be rounded to thei… #13613

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

WalterBright
Copy link
Member

…r precision

As @ibuclaw pointed out, this can be forced already with core.math.toPrec, but I suspect that making enum, const, and static all produce the same value should be unsurprising.

Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

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

Using native types won't work for emulated floating point. This operation should really go via CTFloat, so that each compiler (or longdouble type) can implement it in the correct way for them.

@WalterBright
Copy link
Member Author

I'm not sure why it wouldn't work. The D spec requires float and double to be IEEE, and the D compiler is compiled with a D compiler.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
22740 enhancement float and double literals should be rounded to their precision

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#13613"

@WalterBright
Copy link
Member Author

It appears that CTFloat cannot currently convert to float or double. I've been considering writing an 80 bit emulator, it seems this PR will have to wait for that.

@WalterBright
Copy link
Member Author

I haven't been able to find online a Boost compatible 80 bit emulator.

@ibuclaw
Copy link
Member

ibuclaw commented Feb 6, 2022

I'm not sure why it wouldn't work. The D spec requires float and double to be IEEE, and the D compiler is compiled with a D compiler.

But the longdouble implementation is not necessarily real, and doesn't have the ability to convert from struct to native.

There's a reason why I've been periodically changing 1.0 and 0.0 to CTFloat.one and CTFloat.zero in the frontend whenever native float has accidentally crept in from time to time.

@ibuclaw
Copy link
Member

ibuclaw commented Feb 6, 2022

It appears that CTFloat cannot currently convert to float or double. I've been considering writing an 80 bit emulator, it seems this PR will have to wait for that.

Ah, so it isn't just gdc that emulates a float then judging from the Win64 failures. Good to know.

@ibuclaw
Copy link
Member

ibuclaw commented Feb 6, 2022

I haven't been able to find online a Boost compatible 80 bit emulator.

Can't you just add a CTFloat.truncate function, rather than go the whole hog?

@kinke
Copy link
Contributor

kinke commented Feb 6, 2022

This looks inferior to #11387 to me - parsing the literals as real_t and then converting to float/double, instead of parsing them directly as float/double, yielding the same values a C[++] compiler would. Edit: And with a proper overflow check.

And AFAICT it doesn't address the concerns raised in my PR.

@kinke
Copy link
Contributor

kinke commented Feb 6, 2022

But the longdouble implementation is not necessarily real, and doesn't have the ability to convert from struct to native.

I think all real_t types should have the ability to be cast to a double/float. Like

T opCast(T)() const @trusted
{
static if (is(T == bool)) return mantissa != 0 || (exp_sign & 0x7fff) != 0;
else static if (is(T == byte)) return cast(T)ld_read(&this);
else static if (is(T == ubyte)) return cast(T)ld_read(&this);
else static if (is(T == short)) return cast(T)ld_read(&this);
else static if (is(T == ushort)) return cast(T)ld_read(&this);
else static if (is(T == int)) return cast(T)ld_read(&this);
else static if (is(T == uint)) return cast(T)ld_read(&this);
else static if (is(T == float)) return cast(T)ld_read(&this);
else static if (is(T == double)) return cast(T)ld_read(&this);
else static if (is(T == long)) return ld_readll(&this);
else static if (is(T == ulong)) return ld_readull(&this);
else static if (is(T == real))
{
// convert to front end real if built with dmd
if (real.sizeof > 8)
return *cast(real*)&this;
else
return ld_read(&this);
}
else static assert(false, "usupported type");
}
.

@ibuclaw
Copy link
Member

ibuclaw commented Feb 6, 2022

But the longdouble implementation is not necessarily real, and doesn't have the ability to convert from struct to native.

I think all real_t types should have the ability to be cast to a double/float. Like

T opCast(T)() const @trusted
{
static if (is(T == bool)) return mantissa != 0 || (exp_sign & 0x7fff) != 0;
else static if (is(T == byte)) return cast(T)ld_read(&this);
else static if (is(T == ubyte)) return cast(T)ld_read(&this);
else static if (is(T == short)) return cast(T)ld_read(&this);
else static if (is(T == ushort)) return cast(T)ld_read(&this);
else static if (is(T == int)) return cast(T)ld_read(&this);
else static if (is(T == uint)) return cast(T)ld_read(&this);
else static if (is(T == float)) return cast(T)ld_read(&this);
else static if (is(T == double)) return cast(T)ld_read(&this);
else static if (is(T == long)) return ld_readll(&this);
else static if (is(T == ulong)) return ld_readull(&this);
else static if (is(T == real))
{
// convert to front end real if built with dmd
if (real.sizeof > 8)
return *cast(real*)&this;
else
return ld_read(&this);
}
else static assert(false, "usupported type");
}
.

That only works because the underlying layout matches native reals though. Which isn't the case for me.

@kinke
Copy link
Contributor

kinke commented Feb 6, 2022

That only works because the underlying layout matches native reals though. Which isn't the case for me.

So IIUC, the type you are using cannot be simply converted to a native double or float?! I know for a fact that LLVM's APFloat (arbitrary-precision float) can: https://llvm.org/doxygen/classllvm_1_1APFloat.html#a37733c4c22afc6a48194783dbd25487c

@ibuclaw
Copy link
Member

ibuclaw commented Feb 6, 2022

That only works because the underlying layout matches native reals though. Which isn't the case for me.

So IIUC, the type you are using cannot be simply converted to a native double or float?! I know for a fact that LLVM's APFloat (arbitrary-precision float) can: https://llvm.org/doxygen/classllvm_1_1APFloat.html#a37733c4c22afc6a48194783dbd25487c

I see real_to_target in the headers, that writes the result to a c_long array that may or may not be punnable to native. I don't really want to find out, as it would surely be more trivial doing this on the emulated type directly.

The number of times a float or double appears in gdc is zero, so it would be good to keep it that way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants