feat: add `napi/argv-complex64` #1760

Rejoan-Sardar · 2024-03-07T18:06:11Z

Resolves #817 .

Description

What is the purpose of this pull request?

This pull request C APIs for converting a JavaScript Complex64 object instance to a native C data type stdlib_complex64_t

Related Issues

Does this pull request have any related issues?

This pull request:

resolves [RFC]: Add @stdlib/napi/argv-complex64 #817

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.

Read, understood, and followed the [contributing guidelines][contributing].

@stdlib-js/reviewers

Pranavchiku

Requested changes, once addressed this PR can be reviewed.

lib/node_modules/@stdlib/napi/argv-complex64/binding.gyp

Pranavchiku · 2024-03-08T10:19:32Z

lib/node_modules/@stdlib/napi/argv-complex64/include.gypi

@@ -0,0 +1,53 @@
+# @license Apache-2.0
+#
+# Copyright (c) 2022 The Stdlib Authors.


Suggested change

# Copyright (c) 2022 The Stdlib Authors.

# Copyright (c) 2024 The Stdlib Authors.

Pranavchiku · 2024-03-08T10:20:44Z

lib/node_modules/@stdlib/napi/argv-complex64/include/stdlib/napi/argv_complex64.h

+
+#include "stdlib/napi/argv.h"
+#include "stdlib/assert/napi/status_ok.h"
+#include "stdlib/complex/float32.h"


why do we need this?

@Pranavchiku I saw that some of argv package already used these that's why I used in my implemented package.
If they are of no use, I will remove them

Pranavchiku · 2024-03-08T10:20:51Z

lib/node_modules/@stdlib/napi/argv-complex64/include/stdlib/napi/argv_complex64.h

+* @example
+* #include "stdlib/napi/argv_complex64.h"
+* #include "stdlib/napi/argv.h"
+* #include "stdlib/complex/float32.h"


same question

Pranavchiku · 2024-03-08T10:21:16Z

lib/node_modules/@stdlib/napi/argv-complex64/include/stdlib/napi/argv_complex64.h

+*/
+#define STDLIB_NAPI_ARGV_COMPLEX64( env, name, argv, index )           \
+	napi_value __STDLIB_NAPI_ARGV_COMPLEX64_ERR_ ## name;                   \
+	stdlib_complex64_t name;                                                                 \


If we are using stdlib_complex64_t we will need to include a particular header, no?

@Rejoan-Sardar You did not address this.

Pranavchiku · 2024-03-08T10:22:35Z

lib/node_modules/@stdlib/napi/argv-complex64/lib/native.js

+* wrapper( x );
+*/


Specify return value.

@Rejoan-Sardar You did not resolve this.

lib/node_modules/@stdlib/napi/argv-complex64/manifest.json

lib/node_modules/@stdlib/napi/argv-complex64/src/addon.c

Rejoan-Sardar · 2024-03-12T12:58:26Z

@Pranavchiku I have addressed all the issues you mentioned in your review. Please take a look at the changes

Signed-off-by: Rejoan Sardar <[email protected]>

kgryte · 2024-03-17T03:44:36Z

lib/node_modules/@stdlib/napi/argv-complex64/include/stdlib/napi/argv_complex64.h

+* #include "stdlib/napi/argv_complex64.h"
+* #include <node_api.h>
+*
+* static stdlib_complex64_t fcn( const stdlib_complex64_t ) {


Suggested change

* static stdlib_complex64_t fcn( const stdlib_complex64_t ) {

* static stdlib_complex64_t fcn( const stdlib_complex64_t v ) {

kgryte · 2024-03-17T03:46:06Z

lib/node_modules/@stdlib/napi/argv-complex64/include/stdlib/napi/argv_complex64.h

+*     stdlib_complex64_t out = fcn( value );
+* }
+*/
+#define STDLIB_NAPI_ARGV_COMPLEX64( env, name, argv, index )           \


As done elsewhere (see https://github.com/stdlib-js/stdlib/blob/develop/lib/node_modules/%40stdlib/napi/argv-double/include/stdlib/napi/argv_double.h), align \ at 80 characters, except for those lines which are longer than 80 chars.

kgryte · 2024-03-17T03:49:31Z

lib/node_modules/@stdlib/napi/argv-complex64/include/stdlib/napi/argv_complex64.h

+#ifndef STDLIB_NAPI_ARGV_COMPLEX64_H
+#define STDLIB_NAPI_ARGV_COMPLEX64_H
+
+#include "stdlib/assert/napi/status_ok.h"


You are missing an include for stdlib/napi/argv.h.

kgryte · 2024-03-17T03:50:27Z

lib/node_modules/@stdlib/napi/argv-complex64/include/stdlib/napi/argv_complex64.h

+* @param index    argument index
+*
+* @example
+* #include "stdlib/napi/argv_complex64.h"


You are missing the same header in this example, as well. Please refer to https://github.com/stdlib-js/stdlib/blob/develop/lib/node_modules/%40stdlib/napi/argv-double/include/stdlib/napi/argv_double.h and investigate accordingly.

kgryte · 2024-03-17T03:51:50Z

lib/node_modules/@stdlib/napi/argv-complex64/lib/native.js

+*
+* @private
+* @param {Complex64} v - input value
+* @returns {stdlib_complex64_t} input value


This is incorrect. We don't embed C types in JavaScript JSDoc.

kgryte · 2024-03-17T03:52:43Z

lib/node_modules/@stdlib/napi/argv-complex64/manifest.json

+	"confs": [
+		{
+			"src": [
+       			"./src/main.c"


You have mixed TAB and space indentation. This file should be 2-space indentation.

kgryte · 2024-03-17T03:54:53Z

lib/node_modules/@stdlib/napi/argv-complex64/src/addon.c

+	STDLIB_NAPI_ARGV( env, info, argv, argc, 1 )
+	STDLIB_NAPI_ARGV_COMPLEX64( env, value, argv, 0 )
+	stdlib_complex64_t out = identity( value );
+	return out;


This is incorrect. out is not a Node-API value.

kgryte · 2024-03-17T03:56:53Z

lib/node_modules/@stdlib/napi/argv-complex64/src/addon.c

+static napi_value addon( napi_env env, napi_callback_info info ) {
+	STDLIB_NAPI_ARGV( env, info, argv, argc, 1 )
+	STDLIB_NAPI_ARGV_COMPLEX64( env, value, argv, 0 )
+	stdlib_complex64_t out = identity( value );


You should also mirror what we did in https://github.com/stdlib-js/stdlib/blob/develop/lib/node_modules/%40stdlib/napi/argv-double/src/addon.c#L47. Namely, this file is intended to verify that we can successfully extract a complex number object. As such, we will be passing in a known value and we should assert that it has expected values for real and imaginary components. You can use @stdlib/complex/realf and @stdlib/complex/imagf to retrieve the individual components.

kgryte · 2024-03-17T03:58:43Z

lib/node_modules/@stdlib/napi/argv-complex64/src/main.c

+		return napi_ok;
+	}
+
+	bool hprop;


You're missing the stdbool.h header in the list of includes above.

kgryte · 2024-03-17T03:59:06Z

lib/node_modules/@stdlib/napi/argv-complex64/src/main.c

+	status = napi_has_named_property( env, value, "re", &hprop );
+	assert( status == napi_ok );
+	if ( !hprop ) {
+		status = napi_throw_type_error( env, NULL, "Invalid argument. The Complex64 object must have a real component." );


Please follow error message conventions.

kgryte · 2024-03-17T04:00:27Z

lib/node_modules/@stdlib/napi/argv-complex64/src/main.c

+		return NULL;
+	}
+
+	single real;


It is clear to me that you did not actually test this implementation. The C type should be float. Similarly, there is no napi_get_value_single API.

@kgryte This was my first PR and on that time I'm not aware lots of thing. After that I'm not go through it now I will revisit it and change accordingly.

kgryte

This PR needs considerable work before it can be merged. I left an initial round of comments.

kgryte · 2024-03-17T04:01:40Z

@Rejoan-Sardar Please ensure you have locally tested before you indicate that a PR is ready for review and please only mark conversations as "resolved" if you have actually resolved the comments.

Rejoan-Sardar · 2024-03-17T07:07:44Z

Thanks for pointing this out @kgryte

feat: @stdlib/napi/argv-complex64

a82e93f

Planeshifter changed the title ~~feat: add @stdlib/napi/argv-complex64~~ feat: add napi/argv-complex64 Mar 7, 2024

Pranavchiku requested changes Mar 8, 2024

View reviewed changes

Pranavchiku added 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. labels Mar 8, 2024

feat: napi/argv-complex64

5324b14

Rejoan-Sardar requested a review from Pranavchiku March 8, 2024 17:50

Update native.js

a145b05

Signed-off-by: Rejoan Sardar <[email protected]>

Rejoan-Sardar closed this Mar 15, 2024

Rejoan-Sardar reopened this Mar 15, 2024

kgryte reviewed Mar 17, 2024

View reviewed changes

kgryte requested changes Mar 17, 2024

View reviewed changes

kgryte mentioned this pull request Mar 17, 2024

feat: add napi/argv-complex128 #1745

Merged

1 task

This was referenced Mar 17, 2024

feat: add C implementation for math/base/special/ahaversin #1655

Closed

[RFC]: add C implementations for base special mathematical functions stdlib-js/google-summer-of-code#42

Closed

Rejoan-Sardar mentioned this pull request Apr 1, 2024

[RFC]: Implement a broader range of statistical distributions stdlib-js/google-summer-of-code#77

Closed

6 tasks

kgryte mentioned this pull request Apr 7, 2024

feat: add blas/base/cscal #2104

Merged

1 task

Rejoan-Sardar closed this by deleting the head repository Apr 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `napi/argv-complex64` #1760

feat: add `napi/argv-complex64` #1760

Rejoan-Sardar commented Mar 7, 2024

Pranavchiku left a comment

Pranavchiku Mar 8, 2024

Pranavchiku Mar 8, 2024

Rejoan-Sardar Mar 8, 2024 •

edited

Loading

Pranavchiku Mar 8, 2024

Pranavchiku Mar 8, 2024

kgryte Mar 17, 2024

Pranavchiku Mar 8, 2024

kgryte Mar 17, 2024

Rejoan-Sardar commented Mar 12, 2024

kgryte Mar 17, 2024

kgryte Mar 17, 2024

kgryte Mar 17, 2024

kgryte Mar 17, 2024

kgryte Mar 17, 2024

kgryte Mar 17, 2024

kgryte Mar 17, 2024

kgryte Mar 17, 2024

kgryte Mar 17, 2024

kgryte Mar 17, 2024

kgryte Mar 17, 2024

Rejoan-Sardar Mar 17, 2024

kgryte left a comment

kgryte commented Mar 17, 2024

Rejoan-Sardar commented Mar 17, 2024

	# Copyright (c) 2022 The Stdlib Authors.
	# Copyright (c) 2024 The Stdlib Authors.

	* static stdlib_complex64_t fcn( const stdlib_complex64_t ) {
	* static stdlib_complex64_t fcn( const stdlib_complex64_t v ) {

feat: add napi/argv-complex64 #1760

feat: add napi/argv-complex64 #1760

Conversation

Rejoan-Sardar commented Mar 7, 2024

Description

Related Issues

Questions

Other

Checklist

Pranavchiku left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rejoan-Sardar Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rejoan-Sardar commented Mar 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgryte left a comment

Choose a reason for hiding this comment

kgryte commented Mar 17, 2024

Rejoan-Sardar commented Mar 17, 2024

feat: add `napi/argv-complex64` #1760

feat: add `napi/argv-complex64` #1760

Rejoan-Sardar Mar 8, 2024 •

edited

Loading