refactor: update math/base/special/log10 to follow FreeBSD version 12.2.0
#2215
Conversation
Thanks, I've made the changes in both |
| if ( ( ( ihx & STDLIB_CONSTANT_FLOAT64_HIGH_WORD_ABS_MASK ) | ilx ) == 0 ) { | ||
| if ( hx < HIGH_MIN_NORMAL_EXP ) { | ||
| // Case: x < 2**-1022 | ||
| if ( ( ( hx & STDLIB_CONSTANT_FLOAT64_HIGH_WORD_ABS_MASK ) | lx ) == 0 ) { |
There was a problem hiding this comment.
@gunjjoshi @Planeshifter Correct me if I'm wrong, but hx is uint32_t here. Bitwise ops are defined for signed integers. Meaning, we should still be using ihx and ilx here and below.
There was a problem hiding this comment.
I see that we do this in log2 (https://github.com/stdlib-js/stdlib/blob/develop/lib/node_modules/%40stdlib/math/base/special/log2/src/main.c#L75), but I am not sure that is correct or works as intended.
There was a problem hiding this comment.
In
, we do the explicit cast toint32_t. IIRC, the failure to explicitly cast words to signed integers resulted in test failures in other packages.
Update: after digging around a bit, looks like all bitwise operators are defined for unsigned integers. The issue arises when mixing signed and unsigned integer values. In that instance, unsigned integers will cast to signed integers which can cause unexpected behavior. My sense is that wherever FreeBSD uses signed integers, we should also use signed integers. In that case, we should use ihx (see https://svnweb.freebsd.org/base/release/12.2.0/lib/msun/src/e_log10.c?view=markup).
There was a problem hiding this comment.
Yes @kgryte, the process of casting hx to a signed integer after these operations seems correct. I'll try to reiterate over this implementation once again, keeping this in mind.
PR-URL: #2294 Ref: #2215 (comment) Co-authored-by: Athan Reines <[email protected]> Reviewed-by: Athan Reines <[email protected]> Signed-off-by: GUNJ JOSHI <[email protected]> Signed-off-by: Athan Reines <[email protected]>
…`12.2.0` PR-URL: stdlib-js#2215 Reviewed-by: Philipp Burckhardt <[email protected]>
PR-URL: stdlib-js#2294 Ref: stdlib-js#2215 (comment) Co-authored-by: Athan Reines <[email protected]> Reviewed-by: Athan Reines <[email protected]> Signed-off-by: GUNJ JOSHI <[email protected]> Signed-off-by: Athan Reines <[email protected]>
Description
This pull request:
Related Issues
None.
Questions
No.
Other
FreeBSD
log1012.2.0: https://svnweb.freebsd.org/base/release/12.2.0/lib/msun/src/e_log10.c?revision=367086&view=markupThis might be useful to review this PR: refactor: update
math/base/special/log2to follow FreeBSD version12.2.0#2172Checklist
@stdlib-js/reviewers