Skip to content

refactor: update blas/ext/base/dasumpw to follow current project conventions #1998

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

Merged
merged 17 commits into from
Mar 26, 2024

Conversation

rajutkarsh07
Copy link
Contributor

Resolves #1470

Description

update blas/ext/base/dasumpw to follow current project conventions

What is the purpose of this pull request?

This pull request:

Related Issues

Does this pull request have any related issues?

This pull request:

  • resolves #
  • fixes #

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 the BLAS Issue or pull request related to Basic Linear Algebra Subprograms (BLAS). label Mar 22, 2024
@rajutkarsh07
Copy link
Contributor Author

@Planeshifter please review this PR

@kgryte kgryte added Enhancement Issue or pull request for enhancing existing functionality. Native Addons Issue involves or relates to Node.js native add-ons. C Issue involves or relates to C. labels Mar 23, 2024
@kgryte kgryte changed the title refactor update blas/ext/base/dasumpw to follow current project conventions refactor: update blas/ext/base/dasumpw to follow current project conventions Mar 23, 2024
Copy link
Member

@Planeshifter Planeshifter left a comment

Choose a reason for hiding this comment

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

Thanks for your PR!

The main changes needed are updating the addon.c file and the manifest.json in order to get it ready for merging.

@rajutkarsh07
Copy link
Contributor Author

@Planeshifter I have made the suggested changes

@Planeshifter Planeshifter added the Needs Changes Pull request which needs changes before being merged. label Mar 23, 2024
Copy link
Member

@Planeshifter Planeshifter left a comment

Choose a reason for hiding this comment

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

@rajutkarsh07 CI checks are currently failing. Looks like we need an additional empty line at L51 of repl.txt and I think c_dasum in the addon.c file has to be replaced with stdlib_strided_dasumpw for tests to pass. I also believe some changes to manifest.json are still outstanding. Hopefully fixing up these issues won't be too burdensome. Thanks for working on this PR!

@rajutkarsh07
Copy link
Contributor Author

@Planeshifter made the changes in manifest.json and addon.c

Signed-off-by: Philipp Burckhardt <[email protected]>
Signed-off-by: Philipp Burckhardt <[email protected]>
Copy link
Member

@Planeshifter Planeshifter left a comment

Choose a reason for hiding this comment

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

After a few additional changes to the manifest.json, CI checks are now passing. Will merge shortly. Thank you @rajutkarsh07 for your contribution!

@Planeshifter Planeshifter merged commit a70e23d into stdlib-js:develop Mar 26, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BLAS Issue or pull request related to Basic Linear Algebra Subprograms (BLAS). C Issue involves or relates to C. Enhancement Issue or pull request for enhancing existing functionality. Native Addons Issue involves or relates to Node.js native add-ons. Needs Changes Pull request which needs changes before being merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC]: refactor blas/ext/base/dasumpw to follow current project conventions
4 participants