Skip to content
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

feat: add forEach method to array/complex64 #1211

Merged
merged 12 commits into from
Dec 24, 2023

Conversation

Jaysukh-409
Copy link
Member

@Jaysukh-409 Jaysukh-409 commented Dec 24, 2023

Description

What is the purpose of this pull request?

This pull request:

  • Adds the typed array forEach method to prototype of Complex64Array

Related Issues

Does this pull request have any related issues?

No.

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

@kgryte kgryte added the Feature Issue or pull request for adding a new feature. label Dec 24, 2023
Signed-off-by: Athan <kgryte@gmail.com>
Signed-off-by: Athan <kgryte@gmail.com>
Signed-off-by: Athan <kgryte@gmail.com>
Signed-off-by: Athan <kgryte@gmail.com>
Signed-off-by: Athan <kgryte@gmail.com>
Comment on lines 150 to 155
t.deepEqual( reinterpret64( out, 0 ), reinterpret64( out, 0 ), 'returns expected value' );
t.end();

function fcn( v ) {
out.push( v );
}
Copy link
Member

@kgryte kgryte Dec 24, 2023

Choose a reason for hiding this comment

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

@Jaysukh-409 I'm not sure what you are trying to test here. I am surprised this test even runs. reinterpret64 is not intended to operate on generic arrays, so providing out does not seem correct.

Copy link
Member

Choose a reason for hiding this comment

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

And also, reinterpret64( out, 0 ), reinterpret64( out, 0 ) --> I'd expect these two things to be equal. You are testing that out equals out.

Signed-off-by: Athan <kgryte@gmail.com>
Signed-off-by: Athan <kgryte@gmail.com>
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.

@Jaysukh-409 Thanks for working on this. A test case needs to be updated. Once resolved, should be ready for merge.

Signed-off-by: Athan <kgryte@gmail.com>
Signed-off-by: Athan <kgryte@gmail.com>
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. Thanks, @Jaysukh-409!

@kgryte kgryte merged commit 3306285 into stdlib-js:develop Dec 24, 2023
7 checks passed
@kgryte kgryte deleted the foreach branch December 24, 2023 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Issue or pull request for adding a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants