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

Improve integer test for random function #1517

Merged
merged 1 commit into from
Sep 3, 2015

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Sep 2, 2015

This should fix #1514 ... maybe it is worth a further look as I expected a smaller rounding error to compensate. Also the checks should probably be implemented more generic so others can re-use them.

@mgreter mgreter self-assigned this Sep 2, 2015
@mgreter mgreter added this to the 3.3 milestone Sep 2, 2015
@am11
Copy link
Contributor

am11 commented Sep 2, 2015

There is probably one related edge case as @chriseppstein described in sass/sass#1819:

a {
  b: (29 / 7 * 7) == 29;
}

should produce:

a {
  b: true; }  /* at present, it generates b: false */

Reminds me of: https://isocpp.org/wiki/faq/newbie#floating-point-arith2 :)

@mgreter mgreter force-pushed the bugfix/int-test-in-rand-fn branch from 469a83b to a45d5c0 Compare September 2, 2015 12:13
@mgreter
Copy link
Contributor Author

mgreter commented Sep 2, 2015

Also added the epsilon test for number values, so the other sample should now also work. As I said, there are probably more edge cases like this, but this should at least fix the most important ones.

@am11
Copy link
Contributor

am11 commented Sep 2, 2015

Thanks! a45d5c0 fixed the aforementioned edge case for equality. 👍

@mgreter mgreter force-pushed the bugfix/int-test-in-rand-fn branch from a45d5c0 to e2fd9e9 Compare September 2, 2015 14:14
mgreter added a commit that referenced this pull request Sep 3, 2015
Improve integer test for `random` function
@mgreter mgreter merged commit 51449ed into sass:master Sep 3, 2015
@mgreter mgreter deleted the bugfix/int-test-in-rand-fn branch September 3, 2015 18:37
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.

Floating point + the random() function
2 participants