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

Allow pack 'D' on all systems with long doubles #18517

Merged
merged 1 commit into from Feb 19, 2021
Merged

Allow pack 'D' on all systems with long doubles #18517

merged 1 commit into from Feb 19, 2021

Conversation

Leont
Copy link
Contributor

@Leont Leont commented Jan 31, 2021

Previously it was only supported if NV itself also was long double, but not when it is either double or __float128.

This intends to fix #18512

@tonycoz
Copy link
Contributor

tonycoz commented Jan 31, 2021

perlfunc documents the old behaviour.

@Leont
Copy link
Contributor Author

Leont commented Jan 31, 2021

perlfunc documents the old behaviour.

Good point, updated that.

@Tux
Copy link
Contributor

Tux commented Feb 1, 2021

+1 - Thank you!

@ilmari
Copy link
Member

ilmari commented Feb 1, 2021

Shouldn't this also depend on NVSIZE >= LONGDBLSIZE?

@Leont
Copy link
Contributor Author

Leont commented Feb 1, 2021

Shouldn't this also depend on NVSIZE >= LONGDBLSIZE?

Why? Using this if that doesn't hold is silly, but not wrong AFAICT.

@ilmari
Copy link
Member

ilmari commented Feb 1, 2021

Shouldn't this also depend on NVSIZE >= LONGDBLSIZE?

Why? Using this if that doesn't hold is silly, but not wrong AFAICT.

I was thinking by analogy with Q/q, but I guess precision loss in floats is less critical than overflow in integers.

@sisyphus
Copy link
Contributor

sisyphus commented Feb 1, 2021

My main concern is that, when NVSIZE < LONGDBLSIZE, people doing eg print unpack "H*", pack "D>", 42.1 will think that they're looking at the byte structure (in hex) of the value 42.1 expressed as a long double.
In fact, IIUC, they'll be looking at the byte structure (in hex) of the double 42.1 cast to a long double.
I guess they'll eventually notice the 2 (or more) terminating zeros that are always present for all values, and work it out from that.

And I think that those running such a perl configuration, who do understand what "D" formatting is providing, will find little (if any) value in it.

I therefore support an NVSIZE > LONGDBLSIZE condition ... but it's not particularly unusual for me to be out of step with the general consensus.

@Leont
Copy link
Contributor Author

Leont commented Feb 4, 2021

My main concern is that, when NVSIZE < LONGDBLSIZE, people doing eg print unpack "H*", pack "D>", 42.1 will think that they're looking at the byte structure (in hex) of the value 42.1 expressed as a long double.

That would be silly of them.

I'm generally not in favor of making code more complex to enable silliness, but I'm also not in favor of complicating it to prevent people from being silly.

@sisyphus
Copy link
Contributor

sisyphus commented Feb 7, 2021

There's a probably a strong historical aspect to my reticence to endorse the proposal of this PR.
I noticed long ago that "D" formatting was not available for perls whose NVSIZE is less than LONGDBLSIZE - and, like @Leont, I realized that it didn't have to be that way.
But I took the absence of the "D" formatting not to be an oversight - rather, I assumed it was because of concerns similar to the ones that I have expressed here.
And, I've always felt that was the right way to have handled it.
Now, that long held view is now being challenged - which is fair enough.

For sure, it would be handy if, on a perl whose nvtype is double, we could view the internal structure of specific values expressed as long doubles or __float128s.
But I don't think this is the proposal being made here.
It's something that can be implemented easily enough in XS/Inline::C and, in fact, is already provided by Math::MPFR:

C:\>perl -V:nvtype
nvtype='double';

C:\>perl -MMath::MPFR="bytes" -wle "print bytes('123.456', 53); # double"
405edd2f1a9fbe77

C:\>perl -MMath::MPFR="bytes" -wle "print bytes('123.456', 64); # 64-bit prec long double"
4005f6e978d4fdf3b646

C:\>perl -MMath::MPFR="bytes" -wle "print bytes('123.456', 113); # 113-bit prec long double or __float128"
4005edd2f1a9fbe76c8b4395810624dd

C:\>perl -MMath::MPFR="bytes" -wle "print bytes('123.456', 2098); # doubledouble"
405edd2f1a9fbe77bceba5e353f7ced9

C:\>

With Math::MPFR, expressions involving overloaded operators can also be accommodated - eg sqrt(2):

C:\>perl -MMath::MPFR="bytes" -wle "print bytes(sqrt(Math::MPFR->new('2')), 53); # double"
3ff6a09e667f3bcd

C:\>perl -MMath::MPFR=":mpfr,bytes" -wle "Rmpfr_set_default_prec(64); print bytes(sqrt(Math::MPFR->new('2')), 64); # 64-bit prec long double"
3fffb504f333f9de6484

C:\>perl -MMath::MPFR=":mpfr,bytes" -wle "Rmpfr_set_default_prec(113); print bytes(sqrt(Math::MPFR->new('2')), 113); # 113-bit prec long double or __float128"
3fff6a09e667f3bcc908b2fb1366ea95

C:\>perl -MMath::MPFR=":mpfr,bytes" -wle "Rmpfr_set_default_prec(2098); print bytes(sqrt(Math::MPFR->new('2')), 2098); # doubledouble"
3ff6a09e667f3bcdbc9bdd3413b26456

Anyway ... if my concerns re the proposal of this PR are "a lone voice", as seems to be the case, then it's fair enough that they be ignored.

@Tux
Copy link
Contributor

Tux commented Feb 16, 2021

What is needed to make @Leont 's patch merged

@Leont
Copy link
Contributor Author

Leont commented Feb 16, 2021

What is needed to make @Leont 's patch merged

I was waiting for more reactions, but if indeed sisyphus is the only one who wants this disabled on nvsize=8 systems I think we should merge it.

@leonerd
Copy link
Contributor

leonerd commented Feb 16, 2021

I am keen to see D be more widely supported; I have no opinion on how it is achieved. If this achieves it, I see no downside to it.

Previously it was only supported if NV also was long double, but not when
it is either double or __float128.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pack/unpack format 'D' unknown when perl built with -Dusequadmath
6 participants