refactor: use constant packages, FI_F for addon and style fixes in math/base/special/ldexpf
#2868
Conversation
| return frac; | ||
| } | ||
| if ( k <= -25 ) { | ||
| if ( exp > 50000 ) { |
There was a problem hiding this comment.
@gunjjoshi I am a bit confused. Why is this branch unreachable? If you can exercise L133, then you should also be able to exercise exp > 50000 by providing a exp value greater than 50000.
There was a problem hiding this comment.
@kgryte This is because, suppose we take exp as 60000. Then, on L116, k will become greater than or equal to exp, i.e., k>=60000. Then, in the very next line, i.e., on L117, the if condition will evaluate to true, as the condition checks k>0xfe, where 0xfe=254. So, the execution will go inside that block, and the result would be returned from L119.
There was a problem hiding this comment.
@gunjjoshi Okay. We actually need to update L116 to emulate 32-bit integer arithmetic. It should be k = (k + exp)|0;. Then you can replicate the scenario where exp is very large and k becomes negative.
There was a problem hiding this comment.
After updating this to according to 32-bit integer arithmetic, we do not need to add the removed block again here, right? I have added the corresponding test to only test.native.js, as we're not having that block here.
There was a problem hiding this comment.
No, you do need to re-add the block as now it is possible to overflow to a negative number.
There was a problem hiding this comment.
For instance, for exp = 3e9, k = -1294967296 in JS, but in C, k = 1.
There was a problem hiding this comment.
@gunjjoshi Can you try recompiling the addon, but setting the fwrapv compiler flag? https://www.gnu.org/software/c-intro-and-ref/manual/html_node/Signed-Overflow.html
There was a problem hiding this comment.
To compile with the flag set, you can use the same approach as we used when checking for fused operations previously.
There was a problem hiding this comment.
I tried:
CFLAGS="-ffp-contract=off -fwrapv" make install-node-addons NODE_ADDONS_PATTERN="math/base/special/ldexpf"
But even with this, it doesn't work. What happens is, when I pass a large value such as 3e9 for exp, and then try to print the value of exp from inside the function, it comes out to be 1. So, the value of exp is being taken as 1 in these cases.
There was a problem hiding this comment.
Hmm...odd. I wonder if I get the same behavior on my machine. I'll try and check locally sometime in the next day or so.
| return fracc; | ||
| } | ||
| if ( k <= -25 ) { | ||
| if ( exp > 50000 ) { |
There was a problem hiding this comment.
Okay. I see why this was in scalbn. The issue is when exp is so large that k integer overflows. Recall that k is int32_t. When k exceeds the maximum positive signed 32-bit integer, the value wraps around. In which case, L92 can generate a negative k when exp is very large. You need to revert this change.
kgryte
added
the
Needs Changes
Planeshifter
left a comment
Planeshifter
left a comment
There was a problem hiding this comment.
Did a review of the PR and it looks all good to me. All comments have been addressed.
Planeshifter
added
Ready To Merge
PR Commit MessagePlease review the above commit message and make any necessary adjustments. |
4 participants
Description
This pull request:
FI_Finaddon.c.100%test coverage, I tried different values, but there wasn't any pair of arguments for which the function reaches this block. So, I removed it in both implementations.Related Issues
None.
Questions
No.
Other
No.
Checklist
@stdlib-js/reviewers