Skip to content

Warn when number base conversion loses precision - #22371

Merged
LamentXU123 merged 1 commit into
php:masterfrom
Sjord:warn-on-precision-loss
Jul 29, 2026
Merged

Warn when number base conversion loses precision#22371
LamentXU123 merged 1 commit into
php:masterfrom
Sjord:warn-on-precision-loss

Conversation

@Sjord

@Sjord Sjord commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

_php_math_basetozval is used for base_convert, bindec, hexdec and octdec. It uses an integer or double for the internal representation of the number. When the input number is too large to fit in there, it loses precision. This commit emits a notice when this happens.

Discussion thread: https://news-web.php.net/php.internals/131364

@Sjord

Sjord commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

@mvorisek Could you perhaps review this please?

@Sjord

Sjord commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

@LamentXU123 could you perhaps review this?

Comment thread ext/standard/math.c Outdated
num = num * base + c;
break;
} else {
zend_error(E_NOTICE, "Input number exceeds maximum integer value, precision has been lost in conversion");

@LamentXU123 LamentXU123 Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would prefer to indicate the actual "maximum integer value" the input exceed which will start to cause precision lost. But I am not the code owner tho, what do you all think? cc @arnaud-lb

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could say something like "Input number is larger than PHP_INT_MAX, precision has been lost in conversion"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I changed it. I tried to think of something better but eventually just used what you suggested.

@arnaud-lb arnaud-lb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me, thank you for this!

Sjord pushed a commit to Sjord/php-src that referenced this pull request Jul 29, 2026
_php_math_basetozval is used for base_convert, bindec, hexdec and
octdec. It uses an integer or double for the internal representation of
the number. When the input number is too large to fit in there, it loses
precision. This commit emits a notice when this happens.

Discussion thread: https://news-web.php.net/php.internals/131364
PR: php#22371
@Sjord
Sjord force-pushed the warn-on-precision-loss branch from 4f4162d to 4575883 Compare July 29, 2026 17:59
Sjord added a commit to Sjord/php-src that referenced this pull request Jul 29, 2026
_php_math_basetozval is used for base_convert, bindec, hexdec and
octdec. It uses an integer or double for the internal representation of
the number. When the input number is too large to fit in there, it loses
precision. This commit emits a notice when this happens.

Discussion thread: https://news-web.php.net/php.internals/131364
PR: php#22371
@Sjord
Sjord force-pushed the warn-on-precision-loss branch from 4575883 to e596757 Compare July 29, 2026 18:00
_php_math_basetozval is used for base_convert, bindec, hexdec and
octdec. It uses an integer or double for the internal representation of
the number. When the input number is too large to fit in there, it loses
precision. This commit emits a notice when this happens.

Discussion thread: https://news-web.php.net/php.internals/131364
PR: php#22371
@Sjord
Sjord force-pushed the warn-on-precision-loss branch from e596757 to 5df71d9 Compare July 29, 2026 18:21
@LamentXU123
LamentXU123 merged commit 5eb9bbd into php:master Jul 29, 2026
17 of 18 checks passed
@LamentXU123

Copy link
Copy Markdown
Member

Thank you!

@LamentXU123

LamentXU123 commented Jul 29, 2026

Copy link
Copy Markdown
Member

@Sjord Nits: since people (including me) constantly forget to add the prefix in commits when merging, it will be great if you can add these in your PR title in the future. e.g. ext/standard: Warn when number base conversion loses precision in this PR. It is also good to prepare the NEWS and UPGRADING (if proper) in commits just in case the merger forget (e.g. me, in this PR ;) that I forget to add the UPGRADING entry)

LamentXU123 added a commit that referenced this pull request Jul 29, 2026
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.

3 participants