Skip to content

Commit

Permalink
n-api: remove napi_get_value_string_length()
Browse files Browse the repository at this point in the history 
This API doesn't serve much purpose, and is only likely to cause
confusion and bugs. The intention was that this would return the
number of characters in a string independent of encoding, but
that's not generally useful. In almost all cases, one of the
encoding-specific napi_get_value_string_* APIs is more correct.
(Pass a null buffer if only the encoded length is desired.)

Anyway the current implementation of napi_get_value_string_length()
is technically wrong: it returns the number of 2-byte code units of
the UTF-16 encoding, but there are actually some characters that
are encoded as two UTF-16 code units.

Note the JavaScript String.prototype.length property returns the
number of UTF-16 code units, which may be different from the number
of characters. So, getting the true character count is not common
with JavaScript, and is probably best left to specialized
internationalization libraries.

PR-URL: #12496
Fixes: nodejs/abi-stable-node#226
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
  • Loading branch information
@jasongin @mhdawson
jasongin authored and mhdawson committed Apr 20, 2017
1 parent ce986de commit 468275a
Show file tree
Hide file tree
Showing 4 changed files with 10 additions and 30 deletions.
15 changes: 0 additions & 15 deletions src/node_api.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1719,21 +1719,6 @@ napi_status napi_get_value_bool(napi_env env, napi_value value, bool* result) {
return napi_ok;
}

// Gets the number of CHARACTERS in the string.
napi_status napi_get_value_string_length(napi_env env,
napi_value value,
size_t* result) {
NAPI_PREAMBLE(env);
CHECK_ARG(env, result);

v8::Local<v8::Value> val = v8impl::V8LocalValueFromJsValue(value);
RETURN_STATUS_IF_FALSE(env, val->IsString(), napi_string_expected);

*result = val.As<v8::String>()->Length();

return GET_RETURN_STATUS(env);
}

// Copies a JavaScript string into a LATIN-1 string buffer. The result is the
// number of bytes (excluding the null terminator) copied into buf.
// A sufficient buffer size should be greater than the length of string,
Expand Down
5 changes: 0 additions & 5 deletions src/node_api.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,11 +171,6 @@ NAPI_EXTERN napi_status napi_get_value_bool(napi_env env,
napi_value value,
bool* result);

// Gets the number of CHARACTERS in the string.
NAPI_EXTERN napi_status napi_get_value_string_length(napi_env env,
napi_value value,
size_t* result);

// Copies LATIN-1 encoded bytes from a string into a buffer.
NAPI_EXTERN napi_status napi_get_value_string_latin1(napi_env env,
napi_value value,
Expand Down
14 changes: 7 additions & 7 deletions test/addons-napi/test_string/test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const empty = '';
assert.strictEqual(test_string.TestLatin1(empty), empty);
assert.strictEqual(test_string.TestUtf8(empty), empty);
assert.strictEqual(test_string.TestUtf16(empty), empty);
assert.strictEqual(test_string.Length(empty), 0);
assert.strictEqual(test_string.Utf16Length(empty), 0);
assert.strictEqual(test_string.Utf8Length(empty), 0);

const str1 = 'hello world';
Expand All @@ -19,7 +19,7 @@ assert.strictEqual(test_string.TestUtf16(str1), str1);
assert.strictEqual(test_string.TestLatin1Insufficient(str1), str1.slice(0, 3));
assert.strictEqual(test_string.TestUtf8Insufficient(str1), str1.slice(0, 3));
assert.strictEqual(test_string.TestUtf16Insufficient(str1), str1.slice(0, 3));
assert.strictEqual(test_string.Length(str1), 11);
assert.strictEqual(test_string.Utf16Length(str1), 11);
assert.strictEqual(test_string.Utf8Length(str1), 11);

const str2 = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789';
Expand All @@ -29,7 +29,7 @@ assert.strictEqual(test_string.TestUtf16(str2), str2);
assert.strictEqual(test_string.TestLatin1Insufficient(str2), str2.slice(0, 3));
assert.strictEqual(test_string.TestUtf8Insufficient(str2), str2.slice(0, 3));
assert.strictEqual(test_string.TestUtf16Insufficient(str2), str2.slice(0, 3));
assert.strictEqual(test_string.Length(str2), 62);
assert.strictEqual(test_string.Utf16Length(str2), 62);
assert.strictEqual(test_string.Utf8Length(str2), 62);

const str3 = '?!@#$%^&*()_+-=[]{}/.,<>\'"\\';
Expand All @@ -39,7 +39,7 @@ assert.strictEqual(test_string.TestUtf16(str3), str3);
assert.strictEqual(test_string.TestLatin1Insufficient(str3), str3.slice(0, 3));
assert.strictEqual(test_string.TestUtf8Insufficient(str3), str3.slice(0, 3));
assert.strictEqual(test_string.TestUtf16Insufficient(str3), str3.slice(0, 3));
assert.strictEqual(test_string.Length(str3), 27);
assert.strictEqual(test_string.Utf16Length(str3), 27);
assert.strictEqual(test_string.Utf8Length(str3), 27);

const str4 = '¡¢£¤¥¦§¨©ª«¬­®¯°±²³´µ¶·¸¹º»¼½¾¿';
Expand All @@ -49,7 +49,7 @@ assert.strictEqual(test_string.TestUtf16(str4), str4);
assert.strictEqual(test_string.TestLatin1Insufficient(str4), str4.slice(0, 3));
assert.strictEqual(test_string.TestUtf8Insufficient(str4), str4.slice(0, 1));
assert.strictEqual(test_string.TestUtf16Insufficient(str4), str4.slice(0, 3));
assert.strictEqual(test_string.Length(str4), 31);
assert.strictEqual(test_string.Utf16Length(str4), 31);
assert.strictEqual(test_string.Utf8Length(str4), 62);

const str5 = 'ÀÁÂÃÄÅÆÇÈÉÊËÌÍÎÏÐÑÒÓÔÕÖ×ØÙÚÛÜÝÞßàáâãäåæçèéêëìíîïðñòóôõö÷øùúûüýþ';
Expand All @@ -59,13 +59,13 @@ assert.strictEqual(test_string.TestUtf16(str5), str5);
assert.strictEqual(test_string.TestLatin1Insufficient(str5), str5.slice(0, 3));
assert.strictEqual(test_string.TestUtf8Insufficient(str5), str5.slice(0, 1));
assert.strictEqual(test_string.TestUtf16Insufficient(str5), str5.slice(0, 3));
assert.strictEqual(test_string.Length(str5), 63);
assert.strictEqual(test_string.Utf16Length(str5), 63);
assert.strictEqual(test_string.Utf8Length(str5), 126);

const str6 = '\u{2003}\u{2101}\u{2001}\u{202}\u{2011}';
assert.strictEqual(test_string.TestUtf8(str6), str6);
assert.strictEqual(test_string.TestUtf16(str6), str6);
assert.strictEqual(test_string.TestUtf8Insufficient(str6), str6.slice(0, 1));
assert.strictEqual(test_string.TestUtf16Insufficient(str6), str6.slice(0, 3));
assert.strictEqual(test_string.Length(str6), 5);
assert.strictEqual(test_string.Utf16Length(str6), 5);
assert.strictEqual(test_string.Utf8Length(str6), 14);
6 changes: 3 additions & 3 deletions test/addons-napi/test_string/test_string.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ napi_value TestUtf16Insufficient(napi_env env, napi_callback_info info) {
return output;
}

napi_value Length(napi_env env, napi_callback_info info) {
napi_value Utf16Length(napi_env env, napi_callback_info info) {
size_t argc = 1;
napi_value args[1];
NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));
Expand All @@ -171,7 +171,7 @@ napi_value Length(napi_env env, napi_callback_info info) {
"Wrong type of argment. Expects a string.");

size_t length;
NAPI_CALL(env, napi_get_value_string_length(env, args[0], &length));
NAPI_CALL(env, napi_get_value_string_utf16(env, args[0], NULL, 0, &length));

napi_value output;
NAPI_CALL(env, napi_create_number(env, (double)length, &output));
Expand Down Expand Up @@ -209,7 +209,7 @@ void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
DECLARE_NAPI_PROPERTY("TestUtf8Insufficient", TestUtf8Insufficient),
DECLARE_NAPI_PROPERTY("TestUtf16", TestUtf16),
DECLARE_NAPI_PROPERTY("TestUtf16Insufficient", TestUtf16Insufficient),
DECLARE_NAPI_PROPERTY("Length", Length),
DECLARE_NAPI_PROPERTY("Utf16Length", Utf16Length),
DECLARE_NAPI_PROPERTY("Utf8Length", Utf8Length),
};

Expand Down

0 comments on commit 468275a

Please sign in to comment.