-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
add remaining math methods (#524) #525
add remaining math methods (#524) #525
Conversation
Codecov Report
@@ Coverage Diff @@
## master #525 +/- ##
==========================================
+ Coverage 68.10% 68.46% +0.35%
==========================================
Files 169 169
Lines 9942 10080 +138
==========================================
+ Hits 6771 6901 +130
- Misses 3171 3179 +8
Continue to review full report at Codecov.
|
boa/src/builtins/math/mod.rs
Outdated
.and_then(|x| f64::from(x).to_u32()) | ||
.map_or(32usize, |u| { | ||
let s = format!("{:032b}", u); | ||
s.chars().take_while(|x| *x == '0').count() | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.and_then(|x| f64::from(x).to_u32()) | |
.map_or(32usize, |u| { | |
let s = format!("{:032b}", u); | |
s.chars().take_while(|x| *x == '0').count() | |
}) | |
.and_then(|x| i32::from(x).leading_zeros()) | |
.unwrap_or(32) |
A better way of doing this would we to use .leading_zeros()
which would eliminate the allocation from format!
Math.clz32
method (#524)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I'd write some more things on the tests, just to be sure. I know your implementation is correct, but maybe a new implementation in a future refactor might not be. Either way, looks pretty good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks perfect! The additional tests would be nice :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!!
This Pull Request fixes/closes #524.
It changes the following (work-in-progress):