-
-
Notifications
You must be signed in to change notification settings - Fork 777
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
refactor: update math/base/special/rcbrt
#2189
Conversation
Signed-off-by: GUNJ JOSHI <[email protected]>
Signed-off-by: GUNJ JOSHI <[email protected]>
"libraries": [ | ||
"-lm" | ||
], | ||
"libraries": [], |
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.
This is likely fine, as we explicitly specify -lm
in the make
recipes.
y = rcbrt( x[i] ); | ||
if ( y === expected[i] ) { | ||
t.equal( y, expected[i], 'x: '+x[i]+', y: '+y+', expected: '+expected[i] ); | ||
y = rcbrt( x[ i ] ); |
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.
These changes are fine. Note, however, that, for the most part, we are not militant about this. By default, yes, we use spaces. But we also allow some flexibility depending on what aspects of the code should be visually emphasized. Can describe in more detail our "philosophy", if it can be called that, during a call. But regardless, I wouldn't go overboard with going out of your way to enforce spacing, so long as a file is consistent (e.g., nothing along the lines of [i ]
or [ i]
, etc). Otherwise, you're likely to just create more work for yourself.
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.
Okay. I understood this now. I was a bit doubtful about this, that in most of our packages, we do not have spaces here, which seems completely reasonable now.
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.
LGTM. Thanks, @gunjjoshi!
Description
This pull request:
main.c
instead of earlierrcbrt.c
.javascript
examples.Related Issues
None.
Questions
No.
Other
Made changes according to what we did in
rcbrtf
. Along with that, renamedrcbrt.c
tomain.c
to achieve standardization.Checklist
@stdlib-js/reviewers