[Webkit-unassigned] [Bug 38890] Multi-byte issue in JavaScriptCore formatLocaleDate

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=38890


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #55949|review?                     |review+
               Flag|                            |




--- Comment #11 from Darin Adler <darin at apple.com>  2010-05-13 13:21:09 PST ---
(From update of attachment 55949)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.