[webkit-reviews] review granted: [Bug 38890] Multi-byte issue in JavaScriptCore formatLocaleDate : [Attachment 55949] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Leo Yang
<leo.yang at torchmobile.com.cn>'s request for review:
Bug 38890: Multi-byte issue in JavaScriptCore formatLocaleDate
https://bugs.webkit.org/show_bug.cgi?id=38890

Attachment 55949: patch
https://bugs.webkit.org/attachment.cgi?id=55949&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Looking better!

> +#ifdef __STDC_ISO_10646__

This seems like an unusual way to detect the presence of the mbstowcs function
along with the fact that wide characters are Unicode. Is this really defined
consistently everywhere we need to use mbstowcs and nowhere we don't?

> +    // Convert multi-byte result to unicode.

Unicode should be capitalized, since it's a proper noun.

> +	   for (size_t i = 0; i < length; ++i)
> +	       buffer[i] = static_cast<UChar>(tempbuffer[i]);

This needs a comment saying this assumes the characters are all representable
by a single UTF-16 code point. That is not a safe assumption in general but we
are probably OK for all the cases we will encounter in practice. If any
character is a non-BMP one, we'll give the wrong result.

I suggest you also add a comment explaining that if mbstowcs fails we just fall
back on using the 8-bit characters as-is. And explain why and when that is ever
the right thing to do. I think it's never helpful, myself, and it seems a
little strange to leave that code path in there.

I'll say r=me but I'd appreciate follow-up on the issues I mentioned here.