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.

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

Jump to bottom

fix Issue 3632 - modify float is float to do a bitwise compare #13780

Merged
merged 2 commits into from
Apr 6, 2022
Merged

fix Issue 3632 - modify float is float to do a bitwise compare #13780

merged 2 commits into from
Apr 6, 2022

Conversation

dkorpel
Copy link
Contributor

@dkorpel dkorpel commented Mar 8, 2022

Reboot of #7568

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @dkorpel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
✓ 3632 enhancement modify float is float to do a bitwise compare

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#13780"

@dkorpel dkorpel force-pushed the float-bitwise branch 2 times, most recently from cb7be26 to 021061c Compare March 8, 2022 11:24
@dkorpel
Copy link
Contributor Author

dkorpel commented Mar 8, 2022

Currently fails on Windows:

test28_0.obj : fatal error LNK1179: invalid or corrupt file: duplicate COMDAT '_D4test__T3T46VdeNANZQld'

That's double test.T46!(real.nan).T46, relevant code:

template T46(double v)
{
    double T46 = v;
}

void test46()
{
    double g = T46!(double.nan) + T46!(-double.nan);
}

maxhaton
maxhaton reviewed Mar 8, 2022
View reviewed changes
src/dmd/e2ir.d Outdated
@@ -2024,7 +2024,11 @@ extern (C++) class ToElemVisitor : Visitor
elem *es2 = toElem(ie.e2, irs);
es2 = addressElem(es2, ie.e2.type);
e = el_param(es1, es2);
elem *ecount = el_long(TYsize_t, t1.size());
elem *ecount;
if (t1.ty == TY.Tfloat80)
Copy link
Member

@maxhaton maxhaton Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This could be a ternary + comment and link to issue appreciated since the glue code is horrible as it is.

@dkorpel dkorpel marked this pull request as ready for review March 30, 2022 14:21
@dkorpel dkorpel requested a review from UplinkCoder as a code owner March 30, 2022 14:21
@dkorpel
Copy link
Contributor Author

dkorpel commented Mar 30, 2022

Ready for review now. It does not fix constant folding yet, that's now a separate issue:
Issue 22937 - identity comparison of strings / struct literals not constant folded

It also doesn't change complex numbers, because those are deprecated any way.

@thewilsonator
Copy link
Contributor

cc @kinke does LDC still have that real padding issue?

@kinke
Copy link
Contributor

kinke commented Mar 31, 2022 •
edited
Loading

The padding is still undefined (edit: for real scalars; it's well-defined/zero for aggregate fields), but is is restricted to the actual payload (I think I've fixed that quite some years ago...).

kinke
kinke reviewed Mar 31, 2022
View reviewed changes
src/dmd/ctfeexpr.d Outdated
@@ -1408,6 +1408,34 @@ bool ctfeIdentity(const ref Loc loc, EXP op, Expression e1, Expression e2)
complex_t v2 = e2.toComplex();
cmp = RealIdentical(creall(v1), creall(v2)) && RealIdentical(cimagl(v1), cimagl(v1));
}
else if (e1.op == EXP.structLiteral || e2.op == EXP.structLiteral)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's all this struct literal business, does it have anything to do with floating-point identity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, I did not originally write it, I rebased Iain's PR which rebased another PR from 2013. I removed it and modified ctfeRawCmp now.

@RazvanN7 RazvanN7 requested a review from ibuclaw April 5, 2022 08:31
@RazvanN7 RazvanN7 mentioned this pull request Apr 5, 2022
Closed
@dkorpel dkorpel merged commit eb51f15 into dlang:master Apr 6, 2022
@dkorpel dkorpel deleted the float-bitwise branch April 6, 2022 09:19
@ibuclaw ibuclaw mentioned this pull request Dec 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxhaton maxhaton maxhaton left review comments

@kinke kinke kinke left review comments

@RazvanN7 RazvanN7 RazvanN7 approved these changes

@thewilsonator thewilsonator thewilsonator approved these changes

@UplinkCoder UplinkCoder Awaiting requested review from UplinkCoder UplinkCoder is a code owner

@ibuclaw ibuclaw Awaiting requested review from ibuclaw

Assignees
No one assigned
Labels
Severity:Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dkorpel @dlang-bot @thewilsonator @kinke @maxhaton @RazvanN7