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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue May 11 10:51:42 PDT 2010


Darin Adler <darin at apple.com> has denied 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 55674: patch
https://bugs.webkit.org/attachment.cgi?id=55674&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
What's the guarantee that wide characters are UTF-16? This whole patch seems to
be based on that assumption. There needs to be at least a comment indicating
why that's so.

> +    // Convert multibyte result to wide char

Sentence comments should use whole words and periods. So please call this "wide
characters" and use a period.

> +    UChar buffer[128];

Would be better to the value bufsize that is already here instead of repeating
the 128 here.

> +    size_t len;

We don't abbreviate variable names like this.

> +    // Because sizeof can't be used during preprocessing, we do runtime
check.
> +    if (sizeof(UChar) == sizeof(wchar_t))
> +	   len = mbstowcs((wchar_t*)buffer, timebuffer, sizeof(buffer) /
sizeof(buffer[0]) - 1);

We don't use C-style casts, so this should be reinterpret_cast.

Do we really need the optimized case? It seems fine to always copy from a
wchar_t buffer into a UChar buffer, so I suggest removing the special case
loop.

> +	   wchar_t tempbuffer[128];

Would be better to use bufsize instead of repeating the 128 here. We normally
don't ram words together. The existing name "timebuffer" is not our WebKit
style. I'd call this "wideCharacterBuffer".

> +	   len = mbstowcs(tempbuffer, timebuffer, sizeof(tempbuffer) /
sizeof(tempbuffer[0]) -1);

There should be a space after the "-" here.

> +	   for (size_t i = 0; i < len && len != (size_t)(-1); ++i)
> +	       buffer[i] = (UChar)tempbuffer[i];

Checking that length is not -1 every time through the loop doesn't make sense.
That should be a separate if statement.

We don't use C-style casts, so this should be no cast at all, or a cast to
UChar if the cast is needed for some reason.

> +    if (UNLIKELY(len == (size_t)(-1)))

This should be static_cast.

There's no need to use UNLIKELY for this, because it's not so
performance-critical that we need to optimize that much. The work here dwarfs
that single patch.

review- because of these minor issues, but there also is the issue that wchar_t
is not always UTF-16, which seems like a major one.


More information about the webkit-reviews mailing list