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

fix string comparisons with $] to use numeric comparison instead #464

Closed
wants to merge 1 commit into from

Conversation

book
Copy link
Member

@book book commented Dec 13, 2024

The fix follows Zefram's suggestion from
https://www.nntp.perl.org/group/perl.perl5.porters/2012/05/msg186846.html

On older perls, however, $] had a numeric value that was built up using
floating-point arithmetic, such as 5+0.006+0.000002. This would not
necessarily match the conversion of the complete value from string form
[perl #72210]. You can work around that by explicitly stringifying
$] (which produces a correct string) and having that numify (to a
correctly-converted floating point value) for comparison. I cultivate
the habit of always stringifying $] to work around this, regardless of
the threshold where the bug was fixed. So I'd write

use if "$]" >= 5.014, warnings => "non_unicode";

This ensures that the comparisons will still work when Perl's major version changes to anything greater than 9.

The fix follows Zefram's suggestion from
https://www.nntp.perl.org/group/perl.perl5.porters/2012/05/msg186846.html

> On older perls, however, $] had a numeric value that was built up using
> floating-point arithmetic, such as 5+0.006+0.000002.  This would not
> necessarily match the conversion of the complete value from string form
> [perl #72210].  You can work around that by explicitly stringifying
> $] (which produces a correct string) and having *that* numify (to a
> correctly-converted floating point value) for comparison.  I cultivate
> the habit of always stringifying $] to work around this, regardless of
> the threshold where the bug was fixed.  So I'd write
>
>     use if "$]" >= 5.014, warnings => "non_unicode";

This ensures that the comparisons will still work when Perl's major
version changes to anything greater than 9.
@Grinnz
Copy link
Contributor

Grinnz commented Dec 13, 2024

The changes don't seem to match the description here

@book
Copy link
Member Author

book commented Dec 13, 2024

The changes don't seem to match the description here

Indeed. This is extracted from a larger coming on perl/perl5 that I'm splitting and sending back to the various upstream repositories.

I'll push the commit again, with a more accurate message.

@haarg
Copy link
Member

haarg commented Dec 13, 2024

Pulling internals out of the version object is definitely wrong.

If perl dropped the 5 from the version, this would also be wrong. Binary compatibility would only be tied to the major version, not the minor version.

@book
Copy link
Member Author

book commented Dec 14, 2024

Pulling internals out of the version object is definitely wrong.

It did feel icky when I wrote it.

Would something like this be better?

$archname .= sprintf "-%d.%d", $^V =~ /v(\d+)\.(\d+)/;

@book
Copy link
Member Author

book commented Dec 14, 2024

If perl dropped the 5 from the version, this would also be wrong. Binary compatibility would only be tied to the major version, not the minor version.

Oh, you mean that for the hypothetical case where $^V is 41.8.0, only 41 should be used?

I think that I can only make that patch when the decision has actually been made. And then it's going to be an additional branch in the if/elsif bloc, anyway.

@book
Copy link
Member Author

book commented Dec 14, 2024

Closing this for now.

@book book closed this Dec 14, 2024
@haarg
Copy link
Member

haarg commented Dec 14, 2024

Oh, you mean that for the hypothetical case where $^V is 41.8.0, only 41 should be used?

Yes. Likely $Config{api_version}. $Config{api_revision} would be either dropped or left in place permanently as "5" like PERL_REVISION would be.

@haarg
Copy link
Member

haarg commented Dec 14, 2024

Actually, trying to abbreviate the version here seems wrong. It should just be using $Config{api_versionstring}.

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.

3 participants