Skip to content

Conversation

pmmaga
Copy link
Contributor

@pmmaga pmmaga commented Oct 4, 2017

Link for bugsnet: https://bugs.php.net/bug.php?id=75310

In #1695, this constant was removed. However, in cases like the one described in the bug report, this hack still makes a difference.

@pmmaga pmmaga changed the base branch from master to PHP-7.0 October 5, 2017 08:29
@pmmaga pmmaga changed the base branch from PHP-7.0 to master October 5, 2017 08:29
@cmb69
Copy link
Member

cmb69 commented Oct 5, 2017

If at all, shouldn't we use DBL_EPSILON instead of introducing an own constant?

@pmmaga pmmaga changed the base branch from master to PHP-7.0 October 5, 2017 17:34
@pmmaga pmmaga changed the title Fix #75310 - Reintroduce DOUBLE_DRIFT_FIX constant Fix #75310 - Use DBL_EPSILON to avoid rounding errors in the range function Oct 5, 2017
@pmmaga
Copy link
Contributor Author

pmmaga commented Oct 5, 2017

@cmb69 thanks for the feedback. That makes it better indeed.

@@ -2160,7 +2161,7 @@ PHP_FUNCTION(range)
RANGE_CHECK_DOUBLE_INIT_ARRAY(low, high);

ZEND_HASH_FILL_PACKED(Z_ARRVAL_P(return_value)) {
for (i = 0, element = low; i < size && element >= high; ++i, element = low - (i * step)) {
for (i = 0, element = low; i < size && element >= (high - DBL_EPSILON); ++i, element = low - (i * step)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A more correct approach of this check would look in pseudo code similar to abs(a - b) < threshold.

Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weltling, thanks for the feedback.

I've moved the constant to the LHS as it makes it more readable. Let me know if you prefer it differently.

@weltling
Copy link
Contributor

@pmmaga thanks for checking. Nope, i literally meant to use abs() and the threshold is meant to be DBL_EPSILON. With that, it would be a usual epsilon comparison and one wouldn't have to care about the sign, however i see now it'd possibly require some more patch in the other area like size calculation. The comparison with >= or <= involving epsilon is still tricky with floating points. There are also so called relative epsilon comparisons of floating point, that might be useful in other cases.

I've just tested more, your patch indeed improves the situation with cases as in the tests pushed, but perhaps you could also check cases like range(111111111110.00057, 111111111110.00049, -0.00001). In general, where the fraction part tends to be smaller while the integral part is non zero and possibly big - some issues might still be present. For sure there is no ideal solution for arbitrary floating numbers, perhaps some documentation would be sufficient to describe reliable cases :/ Anyway, i'd recommend not to targed 7.0 with this, as there are for sure some unknown side effects but it would land in the last 7.0 release with the active support so couldn't get any follow ups.

Thanks.

@pmmaga pmmaga changed the base branch from PHP-7.0 to PHP-7.1 February 27, 2018 17:42
@pmmaga
Copy link
Contributor Author

pmmaga commented Jan 28, 2019

Given that the change would not provide a consistent fix for all cases, it's better to leave the choice to users instead of trying (and sometimes failing) to help.

@pmmaga pmmaga closed this Jan 28, 2019
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.

4 participants