what about possibly placing this in lib/internal/buffer-transcode.js and conditionally require()'ing it. purely cosmetic though, to prevent an extra level of indent. or you could just return early. :)

if (!process.binding('config').hasIntl)
  return;

We going to have a complaint about not supporting SharedArrayBuffer for this?

Eventually, perhaps. Not too worried about that for now.

Maybe give an example of transcoding is not possible?

Hm – this could be a ThrowICUError function, right?

This is fine but at some point this might become a member of the MaybeStackBuffer class? I realize that would conflict a bit with the MaybeStackBuffer<UChar> overload, maybe leave a TODO here?

Is that final to residue from editing?

Yeah, slight brain malfunction there I think ;-)

Mhhh this returns a string or a Buffer depending on the target encoding? I don’t think binary-to-text encodings should be allowed here, .toString() is the right method for them.

Yeah, you're right. I'll pull these back out.

Could this use SwapBytes16?

(ditto)

The 1024 seem kind of magic here, although I realize that is largely my fault. 😄 (Not sure if there’s anything to do about that)

Should be fixed now!

Style: s/from_enc/fromEncoding/ and s/to_enc/toEncoding/. Ditto for cnv_from and cnv_to.

I realize you adapted this code from elsewhere but using snprintf() to format the error message will be much more efficient.

Needs a if (e.Empty()) return Local<Value>();.

I think this should be if (!ret.Empty()) buf->Release(); - it's leaking memory now when the buffer can't be created.

Ditto.

Same as above: strict aliasing violation and prone to crashing.

It seems like there is ample opportunity to share code between Ucs2FromUtf8 and Utf8FromUcs2, they are 80% identical.

Perhaps. For now I'm more inclined to keep these separate as it makes finding and tweaking bugs a bit easier. I'll take another pass in a separate PR to condense things down.

Shouldn't this also reset length_?

If you move this into a common header, you might want to give it a slightly less generic name; e.g. SPREAD_BUFFER_ARG.

Just remove the 'defines' block instead of commenting it out.

if you're returning empty handles as an indicator it's probably be more "V8-ish" have the return signature as a MaybeLocal<Value> instead. been trying to do that in other locations myself.

creating Local<Value>'s but there's no HandleScope. if this callback is expected to always be called within an existing HandleScope (like MakeCallback), mind putting a comment at the top. also like MakeCallback (see src/node.h).

if we know this is a v8::Object then can use e.As<Object>(). that's also more explicit that no extra handle is being created.

The new v8::Maybe<T> API for v8::Object::Set() is annoying and ugly, but if if we're going to use some of the new API might as well use all of it.

if you're manipulating the original memory, why bother take a copy?

if this operation fails, do we want to abort or throw?

for future performance enhancement, detect the alignment of the pointer and perform as many swaps that can be done in a single go.

i'm a little confused by this whole object, but right here if we're converting ascii to latin1 shouldn't we be passing 'latin1' as the encoding argument to Buffer.from()?