Skip to content

feat: add C implementation for math/base/special/roundsd #4438

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 25 commits into from
Apr 19, 2025
Merged

feat: add C implementation for math/base/special/roundsd #4438

merged 25 commits into from
Apr 19, 2025

Conversation

@ShabiShett07
Copy link
Contributor

@ShabiShett07 ShabiShett07 commented Jan 1, 2025 •
edited
Loading

Progresses #649 .

Description

What is the purpose of this pull request?

This pull request:

Related Issues

Does this pull request have any related issues?

-progresses #649

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.

@stdlib-js/reviewers

@stdlib-bot stdlib-bot added Math Issue or pull request specific to math functionality. Needs Review A pull request which needs code review. labels Jan 1, 2025
@stdlib-bot
Copy link
Contributor

stdlib-bot commented Jan 1, 2025 •
edited
Loading

Coverage Report

Package Statements Branches Functions Lines
math/base/special/roundsd $\color{green}229/229$
$\color{green}+100.00\%$ 		$\color{green}32/32$
$\color{green}+100.00\%$ 		$\color{green}2/2$
$\color{green}+100.00\%$ 		$\color{green}229/229$
$\color{green}+100.00\%$

The above coverage report was generated for the changes in this PR.

@ShabiShett07
Copy link
Contributor Author

ShabiShett07 commented Jan 2, 2025

/stdlib check-files

@stdlib-bot stdlib-bot added bot: In Progress Pull request is currently awaiting automation. and removed bot: In Progress Pull request is currently awaiting automation. labels Jan 2, 2025
@ShabiShett07
Copy link
Contributor Author

ShabiShett07 commented Jan 2, 2025

/stdlib update-copyright-years

@stdlib-bot stdlib-bot added the bot: In Progress Pull request is currently awaiting automation. label Jan 2, 2025
@stdlib-bot stdlib-bot removed the bot: In Progress Pull request is currently awaiting automation. label Jan 2, 2025
@ShabiShett07
Copy link
Contributor Author

ShabiShett07 commented Jan 2, 2025

/stdlib lint-autofix

@stdlib-bot stdlib-bot added the bot: In Progress Pull request is currently awaiting automation. label Jan 2, 2025
@stdlib-bot stdlib-bot removed the bot: In Progress Pull request is currently awaiting automation. label Jan 2, 2025
@Planeshifter Planeshifter added the Potential Duplicate There might be another pull request resolving the same issue. label Mar 15, 2025
@Planeshifter
Copy link
Member

Planeshifter commented Mar 16, 2025

⚠️ Tracking Issue Closure Warning ⚠️

I noticed your PR description contains closing keywords ("Resolves", "Closes", or "Fixes") referencing a "Tracking Issue".

Why this matters:
Tracking issues should typically remain open until all related sub-issues are completed. GitHub automatically closes issues with such closing keywords when the PR is merged.

Required action:
Use "Progresses:" or "Ref:" instead to reference the tracking issue without automatically closing it.

Thank you for your contribution to the project!

@kgryte kgryte changed the title feat: add math/base/special/roundsd feat: add C implementation for math/base/special/roundsd Apr 2, 2025
@kgryte kgryte added C Issue involves or relates to C. and removed Potential Duplicate There might be another pull request resolving the same issue. labels Apr 2, 2025
@kgryte kgryte requested a review from anandkaranubc April 2, 2025 09:38
anandkaranubc
anandkaranubc requested changes Apr 5, 2025
View reviewed changes
Copy link
Contributor

@anandkaranubc anandkaranubc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ShabiShett07, thanks for your work on this! I've left a number of comments on the PR. There are quite a few areas that need improvement. When you get a chance, please take a look and let me know if anything is unclear. Happy to help clarify!

@anandkaranubc anandkaranubc added Needs Changes Pull request which needs changes before being merged. and removed Needs Review A pull request which needs code review. labels Apr 5, 2025
@kgryte
Copy link
Member

kgryte commented Apr 5, 2025

/stdlib merge

@stdlib-bot stdlib-bot added the bot: In Progress Pull request is currently awaiting automation. label Apr 5, 2025
@stdlib-bot stdlib-bot removed the bot: In Progress Pull request is currently awaiting automation. label Apr 5, 2025
ShabiShett07 and others added 2 commits April 5, 2025 15:27
@ShabiShett07 @anandkaranubc
fix: jsdoc
76f13c5 
Co-authored-by: Karan Anand <119553199+anandkaranubc@users.noreply.github.com>
Signed-off-by: Shabareesh Shetty <139731143+ShabiShett07@users.noreply.github.com>
@ShabiShett07
fix: jsdoc to doxygen
28139c7 
Signed-off-by: Shabareesh Shetty <139731143+ShabiShett07@users.noreply.github.com>
kgryte
kgryte reviewed Apr 18, 2025
View reviewed changes
kgryte
kgryte requested changes Apr 18, 2025
View reviewed changes
Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than some minor nitpicks, this is coming along.

@kgryte kgryte added Needs Changes Pull request which needs changes before being merged. and removed Needs Review A pull request which needs code review. labels Apr 18, 2025
@ShabiShett07
chore: add minor changes
d9eefe3 
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
  - task: lint_filenames
    status: passed
  - task: lint_editorconfig
    status: passed
  - task: lint_markdown
    status: passed
  - task: lint_package_json
    status: passed
  - task: lint_repl_help
    status: passed
  - task: lint_javascript_src
    status: passed
  - task: lint_javascript_cli
    status: na
  - task: lint_javascript_examples
    status: na
  - task: lint_javascript_tests
    status: na
  - task: lint_javascript_benchmarks
    status: na
  - task: lint_python
    status: na
  - task: lint_r
    status: na
  - task: lint_c_src
    status: missing_dependencies
  - task: lint_c_examples
    status: na
  - task: lint_c_benchmarks
    status: na
  - task: lint_c_tests_fixtures
    status: na
  - task: lint_shell
    status: na
  - task: lint_typescript_declarations
    status: passed
  - task: lint_typescript_tests
    status: na
  - task: lint_license_headers
    status: passed
---
@ShabiShett07
Copy link
Contributor Author

ShabiShett07 commented Apr 19, 2025

@kgryte, thank you for the corrections and I have resolved all the conversations

@ShabiShett07 ShabiShett07 requested a review from kgryte April 19, 2025 04:36
@stdlib-bot stdlib-bot added the Needs Review A pull request which needs code review. label Apr 19, 2025
@kgryte
docs: update description
cf8cced 
Signed-off-by: Athan <kgryte@gmail.com>
kgryte
kgryte reviewed Apr 19, 2025
View reviewed changes
@kgryte
docs: fix description
5e16e29 
Signed-off-by: Athan <kgryte@gmail.com>
kgryte
kgryte reviewed Apr 19, 2025
View reviewed changes
@kgryte
docs: fix line wrapping
fa48133 
Signed-off-by: Athan <kgryte@gmail.com>
kgryte
kgryte reviewed Apr 19, 2025
View reviewed changes
@kgryte
docs: fix description
56061f5 
Signed-off-by: Athan <kgryte@gmail.com>
kgryte
kgryte reviewed Apr 19, 2025
View reviewed changes
@kgryte
style: use consistent markup
7790569 
Signed-off-by: Athan <kgryte@gmail.com>
kgryte
kgryte reviewed Apr 19, 2025
View reviewed changes
@kgryte
style: use consistent spacing
4230887 
Signed-off-by: Athan <kgryte@gmail.com>
kgryte
kgryte reviewed Apr 19, 2025
View reviewed changes
@kgryte
style: add missing space
ecf314f 
Signed-off-by: Athan <kgryte@gmail.com>
kgryte
kgryte approved these changes Apr 19, 2025
View reviewed changes
Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after clean-up.

@ShabiShett07 General comment: you'll set yourself up better for long-term success contributing to stdlib if you can pay more attention to detail. In this case, even after updating the descriptions, you ended up diverging from stdlib conventions. When in doubt, study other packages and mimic what you see there. Occasionally, conventions do change, but this is not a frequent occurrence. Currently, a lot of your PRs require a fair amount of clean-up. Would be good to minimize that overhead.

@kgryte kgryte added Feature Issue or pull request for adding a new feature. and removed Needs Changes Pull request which needs changes before being merged. Needs Review A pull request which needs code review. labels Apr 19, 2025
@kgryte kgryte merged commit 9c34a77 into stdlib-js:develop Apr 19, 2025
30 checks passed
@ShabiShett07
Copy link
Contributor Author

ShabiShett07 commented Apr 19, 2025

LGTM after clean-up.

@ShabiShett07 General comment: you'll set yourself up better for long-term success contributing to stdlib if you can pay more attention to detail. In this case, even after updating the descriptions, you ended up diverging from stdlib conventions. When in doubt, study other packages and mimic what you see there. Occasionally, conventions do change, but this is not a frequent occurrence. Currently, a lot of your PRs require a fair amount of clean-up. Would be good to minimize that overhead.

I am really sorry for that, actually didn't give much time to this PR, as I have done this in my initial times, also I think yes, I need to improve a lot in detailing, but thank you for letting me know y mistakes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kgryte kgryte kgryte approved these changes

@anandkaranubc anandkaranubc anandkaranubc left review comments

Assignees

No one assigned

Labels

C Issue involves or relates to C. Feature Issue or pull request for adding a new feature. Math Issue or pull request specific to math functionality.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ShabiShett07 @stdlib-bot @Planeshifter @kgryte @anandkaranubc