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

Don't use PHP's fmod #470

Merged
merged 2 commits into from
May 23, 2018
Merged

Don't use PHP's fmod #470

merged 2 commits into from
May 23, 2018

Conversation

PeterMead
Copy link

This fixes #469.

PHP's fmod has some quirks.  The private fmod function wraps it to iron out some of those quirks but ultimately it's better to just not use it.
Copy link
Contributor

@erayd erayd left a comment

Choose a reason for hiding this comment

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

Good catch - PHP's fmod() loses an unacceptable level of precision.

However.... this whole method is a mess, and most of it is unnecessary. Realistically, all we need to know is whether the remainder is sufficiently close to zero. Which is essentially the following:

$threshold = 0.0000000001;
return abs($value - round($value / $factor) * $factor) < $threshold;

Seeing as that's almost a one-liner, and this method is only called in two places (divisibleBy and multipleOf, which are equivalent anyway), my feeling is that this should be inlined. Seeing as you're fixing the fmod problem anyway, might as well go the whole way and throw out all the other useless junk.

@PeterMead
Copy link
Author

Well I didn't want to criticise the existing code, but yes that function is a bit of a mess. That's mostly because it's trying to work around the issues with PHP's fmod but if we're not using that any more we can simplify the function. I think it's still too complicated for a one liner and if it's used in two places it's better to keep the code in one place.

Now that the private fmod function is not using PHP's fmod we can remove the code which was working around it's quirks.
@bighappyface
Copy link
Collaborator

Per #469 I think it would be nice to have a test demonstrating the false negative.

@bighappyface
Copy link
Collaborator

@erayd should I hold out for a test on this one?

@erayd
Copy link
Contributor

erayd commented May 23, 2018

@bighappyface I'm happy with it as-is. It would be handy to have a test at some point, but I don't think we should be delaying this fix for it, noting the lack of resources.

Copy link
Collaborator

@bighappyface bighappyface left a comment

Choose a reason for hiding this comment

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

+1 for bd87f30
+1 for 1b51bd2

@bighappyface bighappyface merged commit 52086d6 into jsonrainbow:master May 23, 2018
@erayd erayd mentioned this pull request Jan 10, 2019
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.

False negatives with real values of multipleOf
3 participants