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

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from Leo Yang <leo.yang at torchmobile.com.cn>  2010-05-11 20:38:09 PST ---
(In reply to comment #3)
> (From update of attachment 55674 [details])
> 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.
Will follow all above minor issues. 
Yes, wchar_t is not always UTF-16, so I copy the whart_t result to UTF-16 buffer in case of that. In theory, there might be data loss, but datetime related words in different languages should be aligned in to UTF-16; if not, can we handle it by JSString?

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