[webkit-reviews] review granted: [Bug 76647] StringProtoFuncToUpperCase should call StringImpl::upper similar to StringProtoToLowerCase : [Attachment 124834] Updated patch with fix for s-sharp

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Michael Saboff <msaboff at apple.com>'s
request for review:
Bug 76647: StringProtoFuncToUpperCase should call StringImpl::upper similar to
StringProtoToLowerCase
https://bugs.webkit.org/show_bug.cgi?id=76647

Attachment 124834: Updated patch with fix for s-sharp
https://bugs.webkit.org/attachment.cgi?id=124834&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=124834&action=review


> Source/JavaScriptCore/ChangeLog:3
> +2012-01-31  Michael Saboff  <msaboff at apple.com>
> +
> +2012-01-31  Michael Saboff  <msaboff at apple.com>

Double name here.

> Source/JavaScriptCore/ChangeLog:18
> +	   (JSC::stringProtoFuncToLowerCase):

Since all you did was tweak whitespace in this function, I suggest removing
this line.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1206
> +    StringImpl* ourImpl = s.impl();

I would have called this sImpl for consistency with sSize above.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1208
> +    if (ourImpl == upper.get())

The call to .get() here should not be needed.

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:366
> +    const UChar* source16;

This source16 variable declared so long before it’s used is only necessary
because we don’t want to call characters() in the fast 16-bit case. But I think
one more check of is8Bit() is probably cheap enough so we can just call
characters() and avoid it. Either way is probably OK.

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:388
> +	   // There are three special case characters.
> +	   //  1. sharp-S (0xdf) converts to "SS" (two characters)
> +	   //  2. micro (0xb5) converts to upper case micro (0x39c, a 16 bit
character)
> +	   //  3. y umlaut (0xff) converts to upper case Y umlaut (0x178, a 16
bit character)

The code doesn’t assume that there are exactly three of these, so I don’t think
we need to list them. The code simply assumes that there is exactly one
character that converts to two, and that is U+00DF.

I think a better comment would focus on the fact that latinSmallLetterSharpS is
the only character in the range U+0000-U+00FF that converts into more than one
character, because that’s all the code assumes.

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:391
> +	       if (UNLIKELY(c == 0xdf))

Normally we would use a constant named latinSmallLetterSharpS in the header
CharacterNames.h rather than coding 0xdf here in line.

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:395
> +		   // Have a character that is 16bit when converted to
uppercase.

I think you could word this comment better. I would write this:

// Since this upper-cased character does not fit in an 8-bit string, we should
convert to 16-bit.

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:412
> +	       if (UNLIKELY(c == 0xdf)) {

I’m not sure this UNLIKELY is all that helpful.