Skip to content

Conversation

@SwayamInSync
Copy link
Member

closes #153

As per the title

Comment on lines +137 to +139
// we could allow, but this will be bad
// Two values that are different in quad precision,
// might appear equal when converted to double.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// we could allow, but this will be bad
// Two values that are different in quad precision,
// might appear equal when converted to double.
// we could allow this, but it would be bad
// since two values that are different in quad precision
// might appear equal when converted to double.

Copy link
Contributor

@juntyr juntyr left a comment

Choose a reason for hiding this comment

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

Thanks @SwayamInSync for your work! My comments are mostly nits and extra tests, I'll review again afterwards

@SwayamInSync
Copy link
Member Author

New things fixed and added

  • For supporting -nan in strings, dragon4 has to preserve the sign in output to ensure the complete roundtrip
  • Sleef's vendored methods for casting to/from double aren't all supportable over platforms, can cause discripencies so we are explicitly handling those cases.

return Sleef_negq1(QUAD_ZERO); // -0.0
if (Sleef_icmpeqq1(result, QUAD_PRECISION_ZERO)) {
if (Sleef_icmpltq1(*b, QUAD_PRECISION_ZERO)) {
return Sleef_negq1(QUAD_PRECISION_ZERO); // -0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

we have a const for negative zero

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a subtle issues in using that, we didn't used QUAD_PRECISION_NEG_ZERO anywhere because qutil gave the logical representation but the sign information is stored in the actual IEEE 754 binary format, not in the integer literal itself. Basically the sign is encoded in the high mantissa bits. Simply putting a minus sign in front of 0x0 doesn't create a negative zero representation.

I can replace it with a valid representation maybe in future PRs, for now that is just an artifact in the code.

Copy link
Contributor

@juntyr juntyr left a comment

Choose a reason for hiding this comment

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

LGTM with minor nits addressed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use same_value casting in astype

2 participants