n-api: add string api for latin1 encoding and new test cases #12368
Conversation
nodejs-github-bot
added
c++
src/node_api.cc
Outdated
| @@ -1714,6 +1733,39 @@ napi_status napi_get_value_string_length(napi_env env, | |||
| return GET_RETURN_STATUS(env); | |||
| } | |||
|
|
|||
| // Copies a JavaScript string into a LATIN-1 string buffer. The result is the | |||
| // number of bytes copied into buf, including the null terminator. If bufsize | |||
There was a problem hiding this comment.
Now the null terminator is not counted in the result. I understand that is done for consistency with the usage that passes a null buffer just to get the string length. But then the comments need to be updated on all 3 functions.
test/addons-napi/test_string/test.js
Outdated
| assert.strictEqual(test_string.Copy(str4), str4); | ||
| assert.strictEqual(test_string.Length(str4), 3); | ||
| assert.strictEqual(test_string.Utf8Length(str4), 9); | ||
| const str4 = '\u{2003}\u{2101}\u{2001}\u{202}\u{2011}'; |
There was a problem hiding this comment.
Latin1 is not tested here, I assume because these characters are not supported by Latin1 encoding. So then can there be another test case that tests a string with non-ASCII Latin1 characters? (That would also work with UTF8 and UTF16 encodings.)
sampsongao
force-pushed
the
add_latin1_api_and_new_test
branch
from
208bcd4 to
f506579
Compare
There was a problem hiding this comment.
LGTM based on earlier discussion on null termination and with a few requested changes to the comments and subject to adding the test as suggested by Jason. We will need to use our documentation to make it very clear what the returned length means, as otherwise I can see developers using that length to create the buffers they pass in and then having it be truncated to add the terminator.
src/node_api.cc
Outdated
| // number of bytes (excluding the null terminator) copied into buf. | ||
| // If bufsize is insufficient, the string will be truncated and null terminated. | ||
| // If buf is NULL, this method returns the length of the string (in bytes) | ||
| // via the result parameter. |
There was a problem hiding this comment.
Can we clarify here that the length of the string does not include space for the terminator and that you should pass in a buffer of length+1 to avoid truncation ?
src/node_api.cc
Outdated
| // (in 2-byte code units) via the result parameter. | ||
| // number of 2-byte code units (excluding the null terminator) copied into buf. | ||
| // If bufsize is insufficient, the string will be truncated and null terminated. | ||
| // If buf is NULL, this method returns the length of the string (in 2-byte |
There was a problem hiding this comment.
Same comment here as above to explain a buffer of length+1 should be passed in
|
Landed in ad5f987 |
PR-URL: #12368 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]>
PR-URL: nodejs#12368 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]>
Backport-PR-URL: #19447 PR-URL: #12368 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
n-api