[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 #55674|review?                     |review-
               Flag|                            |




--- Comment #3 from Darin Adler <darin at apple.com>  2010-05-11 10:51:43 PST ---
(From update of attachment 55674)
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.

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