[webkit-reviews] review granted: [Bug 17017] Remove KJS::UChar : [Attachment 18754] Combined patch for darin

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Seidel <eric at webkit.org>'s
request for review:
Bug 17017: Remove KJS::UChar
http://bugs.webkit.org/show_bug.cgi?id=17017

Attachment 18754: Combined patch for darin
http://bugs.webkit.org/attachment.cgi?id=18754&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
 552	   if (charLen && (u == 0 || u >= 128 || !strchr(do_not_unescape,
static_cast<unsigned char>(u)))) {

There's no need for a cast here. strchr takes an int and the earlier code on
the line already checks that it's a value in the range 1-127, so a normal
conversion to int will do.

 837		 if (Lexer::isHexDigit((c + 2)[0]) && Lexer::isHexDigit((c +
3)[0]) && Lexer::isHexDigit((c + 4)[0]) && Lexer::isHexDigit((c + 5)[0])) {
 838		     u = Lexer::convertUnicode((c + 2)[0], (c + 3)[0], (c +
4)[0], (c + 5)[0]);

Really should be c[2] instead of (c + 2)[0]. The [0] is a good trick to do this
with a global replace, but it's a little ugly.

 116		 d[i] = static_cast<unsigned char>(c[i]); // static_cast to
unsigned char to avoid erroneous sign-extend

The comment says the sign extend here would be "erroneous", but really I think
that this function is only called on pure ASCII strings. While changing 128-255
into U+0080 to U+00FF is arguably better than changing them to U+FF80 to
U+FFFF, I think what's really going on here is that it's never going to matter.
I think a better comment would simply state that we use unsigned char because
we want to zero-extend rather than sign extending. Note that cross-platform,
plain old "char" can be *either* unsigned or signed.

 97 void Lexer::setCode(int startingLineNumber, const UChar *c, unsigned int
len)

Surprised you didn't move the star.

266212	   UString &append(const char *);
267213	   UString &append(unsigned short);
268214	   UString &append(char c) { return append(static_cast<unsigned
short>(static_cast<unsigned char>(c))); }
269	 UString &append(UChar c) { return append(c.uc); }

I think we need the append(unsigned short) to be changed to be an
append(UChar), because on Windows those two types aren't the same. As-is I
think this might break the Windows build.

r=me on this as-is, but please make sure you don't break the Windows build