Fix #76093: Format strings w/o loss of precision w/ FORMAT_TYPE_DECIMAL #7782
Conversation
|
This is discussed in https://bugs.php.net/bug.php?id=76093 and is also related to a MediaWiki bug described at https://phabricator.wikimedia.org/T268456 . |
| int64_t value = (Z_TYPE_P(number) == IS_DOUBLE)?(int64_t)Z_DVAL_P(number):Z_LVAL_P(number); | ||
| int64_t value; | ||
| if (Z_TYPE_P(number) == IS_DOUBLE) { | ||
| value = (int64_t)Z_DVAL_P(number); |
There was a problem hiding this comment.
AFAIK, this conversion is undefined behavior if the double value is outside the range of int64_t. However, we already do that prior to this patch, so it may be okay for all supported platforms.
There was a problem hiding this comment.
I think you could be justified in broadening the expected test result if platforms really do crazy things with this conversion. But IMO if you've asked for TYPE_INT64 you are explicitly asking for the result of casting the double to the int64, however that's defined on your platform. And as you note, this isn't new behavior in this patch, this is exactly how it worked previously.
There was a problem hiding this comment.
As it turns out, your comment was prescient: the s390 architecture got +9223372036854775808 instead of -9223372036854775808 as a result of that cast (https://app.travis-ci.com/github/php/php-src/jobs/552554610). So I've tweaked the test to allow any value for this case.
cscott
force-pushed
the
bug76093
branch
3 times, most recently
from
34d275c to
d1afe1f
Compare
|
The failing tests seem to be failures of the CI infrastructure, not due to this patch. |
Did you rebase onto current php:master? |
Passing the argument to NumberFormat::format() as a number loses precision if the value can not be represented precisely as a double or long integer. The icu library provides a "decimal number" type that avoids the loss of prevision when the value is passed as a string. Add a new FORMAT_TYPE_DECIMAL to explicitly request the argument be converted to a string and then passed to icu that way.
This extends the argument types of NumberFormat::formatCurrency() to include string, in the same way the earlier patch extended NumberFormat::format() to allow a string. This allows formatting values which can't be represented precisely as a double or long integer, using the "decimal number" type of the icu library.
Ok, just did so. |
|
CI looks better now. :) (usually, only Travis is sometimes flaky) |
|
This change looks good to me. Should this target PHP-8.0, since it's fixing a bug? |
| if( zend_parse_method_parameters( ZEND_NUM_ARGS(), getThis(), "On|l", | ||
| &object, NumberFormatter_ce_ptr, &number, &type ) == FAILURE ) | ||
| { | ||
| RETURN_THROWS(); | ||
| if (object) { | ||
| ZEND_PARSE_PARAMETERS_START(1, 2) | ||
| Z_PARAM_STR_OR_NUMBER(number) | ||
| Z_PARAM_OPTIONAL | ||
| Z_PARAM_LONG(type) | ||
| ZEND_PARSE_PARAMETERS_END(); | ||
| } else { | ||
| ZEND_PARSE_PARAMETERS_START(2, 3) | ||
| Z_PARAM_OBJECT_OF_CLASS(object, NumberFormatter_ce_ptr) | ||
| Z_PARAM_STR_OR_NUMBER(number) | ||
| Z_PARAM_OPTIONAL | ||
| Z_PARAM_LONG(type) | ||
| ZEND_PARSE_PARAMETERS_END(); |
There was a problem hiding this comment.
I think in this case it might make sense to stick with zend_parse_method_parameters(), but replace the n specifier with z, and to replace the EMPTY_SWITCH_DEFAULT_CASE() some lines below to throw a type error manually.
This is more like implementing a feature request, so targeting master seems appropriate. |
| if(sizeof(zend_long) < 8) { | ||
| if (Z_TYPE_P(number) == IS_STRING && type == FORMAT_TYPE_INT64) { | ||
| type = FORMAT_TYPE_DECIMAL; | ||
| } | ||
| } |
There was a problem hiding this comment.
This should better be a compile directive:
| if(sizeof(zend_long) < 8) { | |
| if (Z_TYPE_P(number) == IS_STRING && type == FORMAT_TYPE_INT64) { | |
| type = FORMAT_TYPE_DECIMAL; | |
| } | |
| } | |
| #if SIZEOF_ZEND_LONG < 8 | |
| if (Z_TYPE_P(number) == IS_STRING && type == FORMAT_TYPE_INT64) { | |
| type = FORMAT_TYPE_DECIMAL; | |
| } | |
| #endif |
|
What is the status here? In principle, this PR is fine, but some details should be reconsidered/improved as written above. Also it is not clear if this still can target PHP-8.2; if so, the PR should be rebased onto PHP-8.2 (instead of "master"). |
|
There has not been any recent activity in this PR. It will automatically be closed in 7 days if no further action is taken. |
|
Resubmitted as #9909. |
Passing the argument to NumberFormat::format() as a number loses
precision if the value can not be represented precisely as a
double or long integer. The icu library provides a "decimal number"
type that avoids the loss of prevision when the value is passed
as a string. Add a new FORMAT_TYPE_DECIMAL to explicitly request
the argument be converted to a string and then passed to icu
that way.