From 39b00349b8b44217ffe8244e8e2a451344b42a57 Mon Sep 17 00:00:00 2001 From: Timothy Gu Date: Sun, 19 Feb 2017 20:04:10 -0800 Subject: [PATCH] src, i18n: cleanup usage of MaybeStackBuffer - Templatize AsBuffer() and create a generic version for inclusion in the Buffer class - Use MaybeStackBuffer::storage() - If possible, avoid double conversion in ToASCII()/ToUnicode() - More descriptive assertion error in tests PR-URL: https://github.com/nodejs/node/pull/11464 Reviewed-By: Steven R Loomis Reviewed-By: Anna Henningsen Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell --- src/node_i18n.cc | 110 ++++++++++++++-------------- src/node_internals.h | 29 ++++++++ test/parallel/test-icu-transcode.js | 4 +- 3 files changed, 85 insertions(+), 58 deletions(-) diff --git a/src/node_i18n.cc b/src/node_i18n.cc index 648012a506..ae14aed7c6 100644 --- a/src/node_i18n.cc +++ b/src/node_i18n.cc @@ -72,37 +72,19 @@ using v8::Value; namespace i18n { -const size_t kStorageSize = 1024; - -// TODO(jasnell): This could potentially become a member of MaybeStackBuffer -// at some point in the future. Care would need to be taken with the -// MaybeStackBuffer variant below. -MaybeLocal AsBuffer(Isolate* isolate, - MaybeStackBuffer* buf, - size_t len) { - if (buf->IsAllocated()) { - MaybeLocal ret = Buffer::New(isolate, buf->out(), len); - if (!ret.IsEmpty()) buf->Release(); +template +MaybeLocal ToBufferEndian(Environment* env, MaybeStackBuffer* buf) { + MaybeLocal ret = Buffer::New(env, buf); + if (ret.IsEmpty()) return ret; - } - return Buffer::Copy(isolate, buf->out(), len); -} -MaybeLocal AsBuffer(Isolate* isolate, - MaybeStackBuffer* buf, - size_t len) { - char* dst = reinterpret_cast(**buf); - MaybeLocal ret; - if (buf->IsAllocated()) { - ret = Buffer::New(isolate, dst, len); - if (!ret.IsEmpty()) buf->Release(); - } else { - ret = Buffer::Copy(isolate, dst, len); - } - if (!ret.IsEmpty() && IsBigEndian()) { - SPREAD_BUFFER_ARG(ret.ToLocalChecked(), buf); - SwapBytes16(buf_data, buf_length); + static_assert(sizeof(T) == 1 || sizeof(T) == 2, + "Currently only one- or two-byte buffers are supported"); + if (sizeof(T) > 1 && IsBigEndian()) { + SPREAD_BUFFER_ARG(ret.ToLocalChecked(), retbuf); + SwapBytes16(retbuf_data, retbuf_length); } + return ret; } @@ -138,14 +120,14 @@ void CopySourceBuffer(MaybeStackBuffer* dest, } } -typedef MaybeLocal (*TranscodeFunc)(Isolate* isolate, +typedef MaybeLocal (*TranscodeFunc)(Environment* env, const char* fromEncoding, const char* toEncoding, const char* source, const size_t source_length, UErrorCode* status); -MaybeLocal Transcode(Isolate* isolate, +MaybeLocal Transcode(Environment* env, const char* fromEncoding, const char* toEncoding, const char* source, @@ -162,12 +144,14 @@ MaybeLocal Transcode(Isolate* isolate, ucnv_convertEx(to.conv, from.conv, &target, target + limit, &source, source + source_length, nullptr, nullptr, nullptr, nullptr, true, true, status); - if (U_SUCCESS(*status)) - ret = AsBuffer(isolate, &result, target - &result[0]); + if (U_SUCCESS(*status)) { + result.SetLength(target - &result[0]); + ret = ToBufferEndian(env, &result); + } return ret; } -MaybeLocal TranscodeToUcs2(Isolate* isolate, +MaybeLocal TranscodeToUcs2(Environment* env, const char* fromEncoding, const char* toEncoding, const char* source, @@ -181,11 +165,11 @@ MaybeLocal TranscodeToUcs2(Isolate* isolate, ucnv_toUChars(from.conv, *destbuf, length_in_chars, source, source_length, status); if (U_SUCCESS(*status)) - ret = AsBuffer(isolate, &destbuf, length_in_chars); + ret = ToBufferEndian(env, &destbuf); return ret; } -MaybeLocal TranscodeFromUcs2(Isolate* isolate, +MaybeLocal TranscodeFromUcs2(Environment* env, const char* fromEncoding, const char* toEncoding, const char* source, @@ -200,37 +184,42 @@ MaybeLocal TranscodeFromUcs2(Isolate* isolate, MaybeStackBuffer destbuf(length_in_chars); const uint32_t len = ucnv_fromUChars(to.conv, *destbuf, length_in_chars, *sourcebuf, length_in_chars, status); - if (U_SUCCESS(*status)) - ret = AsBuffer(isolate, &destbuf, len); + if (U_SUCCESS(*status)) { + destbuf.SetLength(len); + ret = ToBufferEndian(env, &destbuf); + } return ret; } -MaybeLocal TranscodeUcs2FromUtf8(Isolate* isolate, +MaybeLocal TranscodeUcs2FromUtf8(Environment* env, const char* fromEncoding, const char* toEncoding, const char* source, const size_t source_length, UErrorCode* status) { *status = U_ZERO_ERROR; - MaybeStackBuffer destbuf; + MaybeStackBuffer destbuf; int32_t result_length; - u_strFromUTF8(*destbuf, kStorageSize, &result_length, + u_strFromUTF8(*destbuf, destbuf.capacity(), &result_length, source, source_length, status); MaybeLocal ret; if (U_SUCCESS(*status)) { - ret = AsBuffer(isolate, &destbuf, result_length * sizeof(**destbuf)); + destbuf.SetLength(result_length); + ret = ToBufferEndian(env, &destbuf); } else if (*status == U_BUFFER_OVERFLOW_ERROR) { *status = U_ZERO_ERROR; destbuf.AllocateSufficientStorage(result_length); u_strFromUTF8(*destbuf, result_length, &result_length, source, source_length, status); - if (U_SUCCESS(*status)) - ret = AsBuffer(isolate, &destbuf, result_length * sizeof(**destbuf)); + if (U_SUCCESS(*status)) { + destbuf.SetLength(result_length); + ret = ToBufferEndian(env, &destbuf); + } } return ret; } -MaybeLocal TranscodeUtf8FromUcs2(Isolate* isolate, +MaybeLocal TranscodeUtf8FromUcs2(Environment* env, const char* fromEncoding, const char* toEncoding, const char* source, @@ -241,20 +230,21 @@ MaybeLocal TranscodeUtf8FromUcs2(Isolate* isolate, const size_t length_in_chars = source_length / sizeof(UChar); int32_t result_length; MaybeStackBuffer sourcebuf; - MaybeStackBuffer destbuf; + MaybeStackBuffer destbuf; CopySourceBuffer(&sourcebuf, source, source_length, length_in_chars); - u_strToUTF8(*destbuf, kStorageSize, &result_length, + u_strToUTF8(*destbuf, destbuf.capacity(), &result_length, *sourcebuf, length_in_chars, status); if (U_SUCCESS(*status)) { - ret = AsBuffer(isolate, &destbuf, result_length); + destbuf.SetLength(result_length); + ret = ToBufferEndian(env, &destbuf); } else if (*status == U_BUFFER_OVERFLOW_ERROR) { *status = U_ZERO_ERROR; destbuf.AllocateSufficientStorage(result_length); u_strToUTF8(*destbuf, result_length, &result_length, *sourcebuf, length_in_chars, status); if (U_SUCCESS(*status)) { - ret = Buffer::New(isolate, *destbuf, result_length); - destbuf.Release(); + destbuf.SetLength(result_length); + ret = ToBufferEndian(env, &destbuf); } } return ret; @@ -320,7 +310,7 @@ void Transcode(const FunctionCallbackInfo&args) { ABORT(); } - result = tfn(isolate, EncodingName(fromEncoding), EncodingName(toEncoding), + result = tfn(env, EncodingName(fromEncoding), EncodingName(toEncoding), ts_obj_data, ts_obj_length, &status); } else { status = U_ILLEGAL_ARGUMENT_ERROR; @@ -431,7 +421,7 @@ int32_t ToUnicode(MaybeStackBuffer* buf, int32_t len = uidna_nameToUnicodeUTF8(uidna, input, length, - **buf, buf->length(), + **buf, buf->capacity(), &info, &status); @@ -440,13 +430,17 @@ int32_t ToUnicode(MaybeStackBuffer* buf, buf->AllocateSufficientStorage(len); len = uidna_nameToUnicodeUTF8(uidna, input, length, - **buf, buf->length(), + **buf, buf->capacity(), &info, &status); } - if (U_FAILURE(status)) + if (U_FAILURE(status)) { len = -1; + buf->SetLength(0); + } else { + buf->SetLength(len); + } uidna_close(uidna); return len; @@ -465,7 +459,7 @@ int32_t ToASCII(MaybeStackBuffer* buf, int32_t len = uidna_nameToASCII_UTF8(uidna, input, length, - **buf, buf->length(), + **buf, buf->capacity(), &info, &status); @@ -474,13 +468,17 @@ int32_t ToASCII(MaybeStackBuffer* buf, buf->AllocateSufficientStorage(len); len = uidna_nameToASCII_UTF8(uidna, input, length, - **buf, buf->length(), + **buf, buf->capacity(), &info, &status); } - if (U_FAILURE(status)) + if (U_FAILURE(status)) { len = -1; + buf->SetLength(0); + } else { + buf->SetLength(len); + } uidna_close(uidna); return len; diff --git a/src/node_internals.h b/src/node_internals.h index f786adcdb3..b68594162b 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -198,6 +198,35 @@ v8::MaybeLocal New(Environment* env, // because ArrayBufferAllocator::Free() deallocates it again with free(). // Mixing operator new and free() is undefined behavior so don't do that. v8::MaybeLocal New(Environment* env, char* data, size_t length); + +// Construct a Buffer from a MaybeStackBuffer (and also its subclasses like +// Utf8Value and TwoByteValue). +// If |buf| is invalidated, an empty MaybeLocal is returned, and nothing is +// changed. +// If |buf| contains actual data, this method takes ownership of |buf|'s +// underlying buffer. However, |buf| itself can be reused even after this call, +// but its capacity, if increased through AllocateSufficientStorage, is not +// guaranteed to stay the same. +template +static v8::MaybeLocal New(Environment* env, + MaybeStackBuffer* buf) { + v8::MaybeLocal ret; + char* src = reinterpret_cast(buf->out()); + const size_t len_in_bytes = buf->length() * sizeof(buf->out()[0]); + + if (buf->IsAllocated()) + ret = New(env, src, len_in_bytes); + else if (!buf->IsInvalidated()) + ret = Copy(env, src, len_in_bytes); + + if (ret.IsEmpty()) + return ret; + + if (buf->IsAllocated()) + buf->Release(); + + return ret; +} } // namespace Buffer } // namespace node diff --git a/test/parallel/test-icu-transcode.js b/test/parallel/test-icu-transcode.js index 0766f8c6d4..6fd72ce3f8 100644 --- a/test/parallel/test-icu-transcode.js +++ b/test/parallel/test-icu-transcode.js @@ -22,9 +22,9 @@ const tests = { for (const test in tests) { const dest = buffer.transcode(orig, 'utf8', test); - assert.strictEqual(dest.length, tests[test].length); + assert.strictEqual(dest.length, tests[test].length, `utf8->${test} length`); for (let n = 0; n < tests[test].length; n++) - assert.strictEqual(dest[n], tests[test][n]); + assert.strictEqual(dest[n], tests[test][n], `utf8->${test} char ${n}`); } {