stdlib icon indicating copy to clipboard operation
stdlib copied to clipboard

refactor: use stdlib `fmod` and `DDD_D` napi function in `math/base/special/wrap`

Open gunjjoshi opened this issue 1 year ago • 4 comments

Description

What is the purpose of this pull request?

This pull request:

  • completes the TODO to remove math.h in the C implementation and use stdlib_base_fmod in math/base/special/wrap.
  • uses DDD_D napi function, instead of writing everything again in addon.c.
  • uses our fmod in js implementation as well, even though the % operator in js works fine, so that we do not have differences in our implementations.

Related Issues

Does this pull request have any related issues?

None.

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

While committing the changes, the C implementation was giving styling errors, indicating that on lines https://github.com/stdlib-js/stdlib/blob/develop/lib/node_modules/%40stdlib/math/base/special/wrap/src/main.c#L52, https://github.com/stdlib-js/stdlib/blob/develop/lib/node_modules/%40stdlib/math/base/special/wrap/src/main.c#L55 and https://github.com/stdlib-js/stdlib/blob/develop/lib/node_modules/%40stdlib/math/base/special/wrap/src/main.c#L58, the conditions were not proper, as we are checking if it is +-0 and assigning it as +0 again. To avoid this, I used stdlib_base_abs in the conditions. Though this error was only coming for C implementation, I applied the changes to the js implementation as well, so that they are close enough.

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

gunjjoshi avatar Aug 20 '24 13:08 gunjjoshi

The error message, due to which I had to use stdlib_base_abs is as follows:

Screenshot 2024-08-20 at 22 23 35

gunjjoshi avatar Aug 20 '24 16:08 gunjjoshi

@gunjjoshi Oh, right, I see. The C compiler isn't accounting for negative zero here. Instead of abs, use is_negative_zero in the condition check.

kgryte avatar Aug 20 '24 20:08 kgryte

You don't need to do anything in the JS version, TMK. So it's fine if the JS and C implementations diverge on this.

kgryte avatar Aug 20 '24 20:08 kgryte

Made the changes, thanks @kgryte!

gunjjoshi avatar Aug 20 '24 23:08 gunjjoshi