Skip to content

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

Closed
ibuclaw wants to merge 4 commits intodlang:masterfrom
ibuclaw:floatshape
Closed

std.math: Add RealRep union for accessing floating point bits via a union.#5825
ibuclaw wants to merge 4 commits intodlang:masterfrom
ibuclaw:floatshape

Conversation

@ibuclaw
Copy link
Member

@ibuclaw ibuclaw commented Oct 29, 2017

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 ibuclaw requested a review from dnadlinger as a code owner October 29, 2017 20:19
@dlang-bot
Copy link
Contributor

dlang-bot commented Oct 29, 2017

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"

@ibuclaw ibuclaw added the math label Oct 29, 2017
@ibuclaw ibuclaw force-pushed the floatshape branch 4 times, most recently from 7bd4423 to a61bcbc Compare October 29, 2017 23:54
@tsbockman
Copy link
Contributor

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.

@ibuclaw
Copy link
Member Author

ibuclaw commented Nov 3, 2017

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.

@tsbockman
Copy link
Contributor

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?

@ibuclaw
Copy link
Member Author

ibuclaw commented Nov 7, 2017

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.

@tsbockman
Copy link
Contributor

That makes sense.

@quickfur
Copy link
Member

ping @ibuclaw

Please rebase?

@ibuclaw
Copy link
Member Author

ibuclaw commented Feb 5, 2018

Rebased.

@wilzbach
Copy link
Contributor

wilzbach commented Apr 6, 2018

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

@ibuclaw
Copy link
Member Author

ibuclaw commented May 1, 2018

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).

@wilzbach
Copy link
Contributor

wilzbach commented May 9, 2019

@ibuclaw would need another rebase.

@LightBender
Copy link
Contributor

PR closed as stalled. @thewilsonator would like to split this into two PR's. One for the new feature and another for the refactoring.

@LightBender LightBender added the Review:Phantom Zone Has value/information for future work, but closed for now label Oct 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

math Merge:Needs Rebase Merge:stalled Review:Needs Work Review:Phantom Zone Has value/information for future work, but closed for now

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants