stdlib-js
refactor: use `stdlib` equivalent instead of built-in, fix indentation in `math/base/special/ldexp`
Description
What is the purpose of this pull request?
This pull request:
- replaces the use of built-in
powwithstdlib_base_pow. - fixes indentation in
example.c.
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.
No.
Checklist
Please ensure the following tasks are completed before submitting this pull request.
- [x] Read, understood, and followed the contributing guidelines.
@stdlib-js/reviewers
This error in examples comes even without applying the changes in this PR. I'll have a look on ldexp to find out the problem.
The error message comes as:
example.c:20:10: fatal error: 'stdlib/math/base/special/ldexp.h' file not found
#include "stdlib/math/base/special/ldexp.h"
even though the header files looks correct.
@gunjjoshi Looks like ldexp is missing from the manifest.json' dependencies in the example configuration.
@gunjjoshi Looks like
ldexpis missing from themanifest.json' dependencies in theexampleconfiguration.
@Planeshifter For other packages too, we do not include the package's path in the example configuration of manifest.json, right? That is included directly from the header file inside the package. Still, I tried after including that, but the condition was the same.
@gunjjoshi Yeah, the package itself shouldn't be listed as a dependency under its own configuration. I was mistakenly under the impression that the failure was caused in a different package's example. Not clear to me what's going on right now... Also seeing failures loading different modules in the CI checks here.
@gunjjoshi @stdlib/number/float32/base/to-word seems wrong as a dependency in manifest.json. We need @stdlib/number/float64/base/to-words, I think.
@Planeshifter It doesn't seem we have used that package anywhere in ldexp. We should remove that.
We have used float64/base/to_words in main.c, but we listed float32/base/to_word in manifest.json. Even after rectifying that, the error persists.
After looking into this a bit more, I noticed that running the C examples for @stdlib/math/base/special/pow is failing as well.
Running the examples with DEBUG=library-manifest:main, it looks to me as if we have a cycle here and @stdlib/utils/library-manifest may not be handling those yet?
cc @kgryte
Coverage Report
| Package | Statements | Branches | Functions | Lines |
|---|---|---|---|---|
| math/base/special/ldexp | $\color{green}279/279$ $\color{green}+100.00\%$ |
$\color{green}18/18$ $\color{green}+100.00\%$ |
$\color{green}2/2$ $\color{green}+100.00\%$ |
$\color{green}279/279$ $\color{green}+100.00\%$ |
The above coverage report was generated for the changes in this PR.
it looks to me as if we have a cycle here and
@stdlib/utils/library-manifestmay not be handling those yet?
That's correct. We cannot handle cyclic dependencies in C implementations. In this case, I think it would be fine to refactor the example in order to avoid the use of math/base/special/pow. Perhaps instead of randomly generating exponents in a loop, we can simply hardcode 3-4 examples (sign, frac, exp, etc) to show roundtripping. Does that sound reasonable, @gunjjoshi?
Updated the C examples. Also, updated benchmarks to follow latest conventions.
The CI failure seems to occur due to the failure in cephes benchmarks, as it is unable to find cephes files.
The CI failure seems to occur due to the failure in cephes benchmarks
@Planeshifter I forget. Did we not already address the Cephes build issue?
PR Commit Message
bench: update benchmarks and examples in `math/base/special/ldexp`
PR-URL: https://github.com/stdlib-js/stdlib/pull/2781
Co-authored-by: stdlib-bot <[email protected]>
Reviewed-by: Athan Reines <[email protected]>
Signed-off-by: Gunj Joshi <[email protected]>
Please review the above commit message and make any necessary adjustments.
As the CI failure is due to an upstream workflow failure and not due to the changes in this PR, I will go ahead and merge.