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

[N-API] napi_get_value_string_length return length as type too small #13458

Closed
dead-claudia opened this issue Jun 4, 2017 · 3 comments
Closed
Labels
doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API.

Comments

@dead-claudia
Copy link

As specified in the N-API documentation, napi_get_value_string_length returns the string length as an int, but the JS spec mandates it to be an unsigned integer at most 253-1. It may be more appropriate here to return a uint64_t instead.

(BTW, I use "return" loosely, in reference to its out parameter.)

@addaleax
Copy link
Member

addaleax commented Jun 4, 2017

That should probably be removed from the API documentation, it’s no longer part of what Node exports. Also, you’re right, and we do use size_t now, which is the appropriate type for this kind of value.

@vsemozhetbyt vsemozhetbyt added the node-api Issues and PRs related to the Node-API. label Jun 4, 2017
@jasongin
Copy link
Member

jasongin commented Jun 5, 2017

Yes, that API was removed because it was ambiguous about the encoding used to determine the length, and it was redundant with napi_get_value_string_utf8/16. The doc is out of date.

See also #13469 about other areas the doc needs updating.

@jasongin jasongin added the doc Issues and PRs related to the documentations. label Jun 5, 2017
@mhdawson
Copy link
Member

mhdawson commented Jun 5, 2017

I'll take a look at updating the docs tomorrow.

mhdawson added a commit to mhdawson/io.js that referenced this issue Jun 6, 2017
addaleax pushed a commit that referenced this issue Jun 10, 2017
PR-URL: #13508
Fixes: #13469
Fixes: #13458
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jason Ginchereau <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Apr 10, 2018
PR-URL: nodejs#13508
Fixes: nodejs#13469
Fixes: nodejs#13458
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jason Ginchereau <[email protected]>
MylesBorins pushed a commit that referenced this issue Apr 16, 2018
Backport-PR-URL: #19447
PR-URL: #13508
Fixes: #13469
Fixes: #13458
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jason Ginchereau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

No branches or pull requests

5 participants