phobos icon indicating copy to clipboard operation
phobos copied to clipboard

std.math: Add RealRep union for accessing floating point bits via a union.

Open ibuclaw opened this issue 6 years ago • 11 comments

As I'd like to see this move forwards, but am in the middle of finishing up work on new ports (making sure that 128-bit is working properly for both IEEE and IBM reals) so can't really commit at the moment.

This is a slightly more conservative version of #4336. As far as I can see, GDC at least produces identical assembly in the conversion from old to new.

Should allow the bigger refactorings in the other PR to be added gracefully. @tsbockman - would you agree?

ibuclaw avatar Oct 29 '17 20:10 ibuclaw

Thanks for your pull request, @ibuclaw!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

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

dub fetch digger
dub run digger -- build "master + phobos#5825"

dlang-bot avatar Oct 29 '17 20:10 dlang-bot

Unless someone has added floating-point union support to DMD since I last checked, this PR has the same problem which stalled mine: it breaks some parts of std.math which currently work in CTFE.

It could be re-written to emulate a union with a struct and properties encapsulating some pointer code, but given that the response to my efforts at extending the CTFE pointer repainting capabilities to support all basic operations was fairly hostile, it didn't seem worth my time.

I think the compiler team needs to pick a long-term solution to the floating-point CTFE issue before it's worth refactoring std.math.

tsbockman avatar Nov 03 '17 22:11 tsbockman

it breaks some parts of std.math which currently work in CTFE.

I don't particularly buy that, as many functions that do pointer casting are not CTFE-able anyway.

Typical getter/setters look like this.

ushort *vu = cast(ushort *)&f;
vu[EXPPOS_SHORT] = 0;

The value of vu[0] might be inferable but any other index certainly isn't.

ibuclaw avatar Nov 03 '17 23:11 ibuclaw

Specifically, this will break isInfinity and isNaN at CTFE for double and float:

import std.math, std.stdio;

void main() {
	enum float x = float.nan;
	enum a = isInfinity(x);
	enum b = isNaN(x);
	writeln(a, ", ", b);

	enum double y = double.infinity;
	enum c = isInfinity(y);
	enum d = isNaN(y);
	writeln(c, ", ", d);
}

Admittedly, these can both be reimplemented in a CTFE-compatible way by the user easily enough:

bool isInfinity(X)(X x) { return x > X.max || x < -X.max; }
bool isNaN(X)(X x) { return x != x; }

But, it is still a breaking change. If that's considered acceptable, then why not just go with my original pull request?

tsbockman avatar Nov 07 '17 20:11 tsbockman

I'm in the middle of testing ieeeQuadruple and ibmExtended, and finding bugs in both. I'd want to make sure that is complete before introducing yours. To make sure we start from a working implementation before the redesign.

ibuclaw avatar Nov 07 '17 21:11 ibuclaw

That makes sense.

tsbockman avatar Nov 08 '17 00:11 tsbockman

ping @ibuclaw

Please rebase?

quickfur avatar Jan 19 '18 19:01 quickfur

Rebased.

ibuclaw avatar Feb 05 '18 22:02 ibuclaw

@ibuclaw maybe it's better if you rebase yourself this time?

wilzbach avatar Apr 06 '18 14:04 wilzbach

Rebased, but I'll have to double-check that ibmExtended is still fine (did some educated guesswork on reimplementing the floorImpl I added in the new format).

ibuclaw avatar May 01 '18 19:05 ibuclaw

@ibuclaw would need another rebase.

wilzbach avatar May 09 '19 16:05 wilzbach