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 napi/argv-complex128 #1445

Conversation

PoookieCoder
Copy link
Contributor

@PoookieCoder PoookieCoder commented Mar 2, 2024

Resolves #816 .

Description

What is the purpose of this pull request?

This pull request adds C APIs for converting a JavaScript Complex128 object instance to a native C data type stdlib_complex128_t.

Related Issues

Does this pull request have any related issues?

This pull request:

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.

After the review of this PR, the #817 issue can be resolved easily. I will open a PR for the same.

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

@PoookieCoder PoookieCoder changed the title feat/@stdlib/napi/argv complex128 feat: add @stdlib/napi/argv-complex128 Mar 2, 2024
@kgryte kgryte added Needs Review A pull request which needs code review. Native Addons Issue involves or relates to Node.js native add-ons. C Issue involves or relates to C. labels Mar 2, 2024
@kgryte kgryte changed the title feat: add @stdlib/napi/argv-complex128 feat: add napi/argv-complex128 Mar 2, 2024
@kgryte kgryte added Feature Issue or pull request for adding a new feature. Utilities Issue or pull request concerning general utilities. labels Mar 2, 2024
Comment on lines 61 to 67
stdlib_assert_napi_value_is_type( env, value, napi_object, message, err );
if ( *err != NULL ) {
return napi_ok;
}
STDLIB_ASSERT_NAPI_STATUS_OK_RET_VALUE( env, napi_get_property( env, value, re, &real ), "", napi_ok )
STDLIB_ASSERT_NAPI_STATUS_OK_RET_VALUE( env, napi_get_property( env, value, im, &imaginary ), "", napi_ok )
*out = stdlib_complex128( real, imaginary );
Copy link
Member

@kgryte kgryte Mar 2, 2024

Choose a reason for hiding this comment

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

This can be improved. E.g., see

napi_value stdlib_math_base_napi_zz_z( napi_env env, napi_callback_info info, stdlib_complex128_t (*fcn)( stdlib_complex128_t, stdlib_complex128_t ) ) {
for reference.

  1. You state that the input value must be a Complex128 instance. We should relax that requirement. We should allow any object coming from JS with a real and imaginary component (may be either an "own" or "inherited" property).
  2. You don't validate that the real and imaginary components are numeric.
  3. You need to unwrap the real and imaginary components before passing to stdlib_complex128. E.g.,
    status = napi_get_value_double( env, xre, &re0 );
    .

@kgryte kgryte added the Needs Changes Pull request which needs changes before being merged. label Mar 2, 2024
aman-095 and others added 18 commits March 7, 2024 07:30
PR-URL: stdlib-js#1443

---------

Signed-off-by: Pranav <[email protected]>
Co-authored-by: Pranav <[email protected]>
Reviewed-by: Pranav <[email protected]>
PR-URL: stdlib-js#1446
Closes: stdlib-js#775

---------

Signed-off-by: Pranav <[email protected]>
Co-authored-by: Pranav <[email protected]>
Reviewed-by: Pranav <[email protected]>
PR-URL: stdlib-js#1430
Closes: stdlib-js#1332

---------

Signed-off-by: Rejoan Sardar <[email protected]>
Signed-off-by: Pranav <[email protected]>
Co-authored-by: Pranav <[email protected]>
Reviewed-by: Pranav <[email protected]>
PR-URL: stdlib-js#1442
Closes: stdlib-js#1432 

---------

Signed-off-by: Pranav <[email protected]>
Co-authored-by: Pranav <[email protected]>
Reviewed-by: Pranav <[email protected]>
PR-URL: stdlib-js#1372
Closes: stdlib-js#1324

Signed-off-by: Utkarsh <[email protected]>
Signed-off-by: Philipp Burckhardt <[email protected]>
Co-authored-by: Philipp Burckhardt <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]> 
Reviewed-by: Athan Reines <[email protected]>
PR-URL: stdlib-js#1462

Signed-off-by: stdlib-bot <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]>
PR-URL: stdlib-js#1461

Signed-off-by: stdlib-bot <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]>
PR-URL: stdlib-js#1654

Signed-off-by: stdlib-bot <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]>
PR-URL: 	stdlib-js#1456
Reviewed-by: Athan Reines <[email protected]>
Signed-off-by: stdlib-bot <[email protected]>
Co-authored-by: Philipp Burckhardt <[email protected]>
PR-URL: stdlib-js#1457
Closes: stdlib-js#1449

---------

Signed-off-by: Pranav <[email protected]>
Co-authored-by: Pranav <[email protected]>
Reviewed-by: Pranav <[email protected]>
PR-URL: stdlib-js#1657
Resolves: stdlib-js#1653 

---------

Signed-off-by: Philipp Burckhardt <[email protected]>
Co-authored-by: Philipp Burckhardt <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]>
PR-URL: stdlib-js#1454 
Closes: stdlib-js#1453

---------

Signed-off-by: Pranav <[email protected]>
Co-authored-by: Pranav <[email protected]>
Reviewed-by: Pranav <[email protected]>
PR-URL: stdlib-js#1667
Closes: stdlib-js#1666

---------

Signed-off-by: Pranav <[email protected]>
Co-authored-by: Pranav <[email protected]>
Reviewed-by: Pranav <[email protected]>
…entions

Closes: stdlib-js#1434
Ref: stdlib-js#1152
PR-URL: 	stdlib-js#1455
Co-authored-by: Athan Reines <[email protected]>
Reviewed-by: Athan Reines <[email protected]> 
Signed-off-by: Snehil Shah <[email protected]>
Signed-off-by: Athan Reines <[email protected]>
PR-URL: 	stdlib-js#1677
Co-authored-by: Athan Reines <[email protected]>
Reviewed-by: Athan Reines <[email protected]> 
Signed-off-by: stdlib-bot <[email protected]>
PR-URL: 	stdlib-js#1676
Co-authored-by: Athan Reines <[email protected]>
Reviewed-by: Athan Reines <[email protected]> 
Signed-off-by: stdlib-bot <[email protected]>
PR-URL: stdlib-js#1348 
Closes: stdlib-js#1337

---------

Signed-off-by: Aditya Sapra <[email protected]>
Signed-off-by: Philipp Burckhardt <[email protected]>
Co-authored-by: Athan Reines <[email protected]>
Co-authored-by: Philipp Burckhardt <[email protected]>
Reviewed-by: Athan Reines <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]>
…egative test values

PR-URL: stdlib-js#1458

Signed-off-by: Snehil Shah <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]>
Shashankss1205 and others added 27 commits March 7, 2024 07:30
Closes: stdlib-js#1668
PR-URL: 	stdlib-js#1670
Co-authored-by: Athan Reines <[email protected]>
Co-authored-by: Pranav Goswami <[email protected]>
Reviewed-by: Athan Reines <[email protected]>
Reviewed-by: Pranav Goswami <[email protected]>
Signed-off-by: Pranav Goswami <[email protected]>
Signed-off-by: Athan Reines <[email protected]>
PR-URL: 	stdlib-js#1709
Co-authored-by: Athan Reines <[email protected]>
Reviewed-by: Athan Reines <[email protected]> 
Signed-off-by: stdlib-bot <[email protected]>
PR-URL: stdlib-js#1685
Closes: stdlib-js#754 

---------

Signed-off-by: Lovelin <[email protected]>
Reviewed-by: Pranav Goswami <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]>
PR-URL: stdlib-js#1447
Closes: stdlib-js#852

---------

Signed-off-by: Anudeep Sanapala <[email protected]>
Signed-off-by: Philipp Burckhardt <[email protected]>
Co-authored-by: stdlib-bot <[email protected]>
Co-authored-by: Philipp Burckhardt <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]>
PR-URL: stdlib-js#1463
Closes: stdlib-js#1327

---------

Signed-off-by: Athan Reines <[email protected]>
Signed-off-by: Aditya Sapra <[email protected]>
Signed-off-by: Philipp Burckhardt <[email protected]>
Co-authored-by: Athan Reines <[email protected]>
Co-authored-by: Philipp Burckhardt <[email protected]>
Reviewed-by: Athan Reines <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]>
PR-URL: stdlib-js#1725

Signed-off-by: stdlib-bot <[email protected]>
Co-authored-by: Philipp Burckhardt <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]>
PR-URL: 	stdlib-js#1726
Reviewed-by: Athan Reines <[email protected]>
Closes: stdlib-js#772
PR-URL: 	stdlib-js#1452
Co-authored-by: Athan Reines <[email protected]>
Co-authored-by: Pranav Goswami <[email protected]>
Reviewed-by: Athan Reines <[email protected]>
Reviewed-by: Pranav Goswami <[email protected]>
Signed-off-by: GUNJ JOSHI <[email protected]>
Signed-off-by: Pranav Goswami <[email protected]>
PR-URL: stdlib-js#1349
Closes: stdlib-js#1321

---------

Signed-off-by: Philipp Burckhardt <[email protected]>
Co-authored-by: Philipp Burckhardt <[email protected]>
Reviewed-by: Athan Reines <[email protected]>
Reviewed-by: Pranav Goswami <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]>
PR-URL: stdlib-js#1719
Closes: stdlib-js#1714

---------

Signed-off-by: Snehil Shah <[email protected]>
Signed-off-by: Pranav <[email protected]>
Co-authored-by: Pranav <[email protected]>
Reviewed-by: Pranav <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]>
PR-URL: stdlib-js#1731
Closes: stdlib-js#1721

---------

Signed-off-by: Snehil Shah <[email protected]>
Co-authored-by: Pranavchiku <[email protected]>
Reviewed-by: Pranavchiku <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]>
PR-URL: stdlib-js#1737

Signed-off-by: stdlib-bot <[email protected]>
Co-authored-by: Planeshifter <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]>
@PoookieCoder PoookieCoder deleted the feature/@stdlib/napi/argv-complex128 branch March 7, 2024 02:06
@Planeshifter Planeshifter removed the Needs Review A pull request which needs code review. label Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C Issue involves or relates to C. Feature Issue or pull request for adding a new feature. Native Addons Issue involves or relates to Node.js native add-ons. Needs Changes Pull request which needs changes before being merged. Utilities Issue or pull request concerning general utilities.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC]: Add @stdlib/napi/argv-complex128