-
Notifications
You must be signed in to change notification settings - Fork 88
Initial nodejs string wrap #151
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
base: main-dev
Are you sure you want to change the base?
Conversation
430f05e
to
f7e4c05
Compare
19e6998
to
4e33434
Compare
e0a9e4e
to
c8c6c7c
Compare
sz_wrapper_t *this; | ||
sz_wrapper_t *arg; | ||
|
||
napi_get_cb_info(env, info, &argc, args, &jsthis, NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling for napi_get_cb_info call. The function returns napi_status which should be checked. If it fails, the function could proceed with invalid args/jsthis values. Should check return value and handle error cases.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
size_t argc = 1; | ||
napi_value args[1]; | ||
napi_value jsthis; | ||
sz_wrapper_t *obj = malloc(sizeof(sz_wrapper_t)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No NULL check after malloc. If allocation fails, the function continues with an invalid pointer which could cause a crash. Should check if obj is NULL and handle the error case appropriately.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
@@ -2,7 +2,7 @@ import test from 'node:test'; | |||
import bindings from 'bindings'; | |||
import assert from 'node:assert'; | |||
|
|||
const stringzilla = bindings('stringzilla'); | |||
const stringzilla = bindings('../../build/Release/stringzilla'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a relative path ('../../build/Release/stringzilla') with the 'bindings' module is incorrect and could cause module resolution failures. The 'bindings' module is designed to automatically locate native addons and should be used with just the module name. The original code using bindings('stringzilla') was correct. The change to a relative path breaks the standard module resolution mechanism and could fail when the package is installed in different locations.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
😱 Found 3 issues. Time to roll up your sleeves! 😱 |
I've created a Str object wrapper for Node.js
And implemented a few functions with tests.