[webkit-reviews] review denied: [Bug 17017] Remove KJS::UChar : [Attachment 18705] send KJS::UChar out to pasture (minus ICU headers)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 18705: send KJS::UChar out to pasture (minus ICU headers)
http://bugs.webkit.org/attachment.cgi?id=18705&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Generally looks good.

+    return toRef(UString(reinterpret_cast<const UChar*>(chars),
static_cast<int>(numChars)).rep()->ref());

+    return toRef(UString(reinterpret_cast<UChar*>(buffer.data()), p -
buffer.data()).rep()->ref());

+  const ::UChar* d = reinterpret_cast<const ::UChar*>(&data()[0]);

+    Completion comp = Interpreter::evaluate(exec, filename, baseLine,
reinterpret_cast<const UChar*>(str.characters()), str.length(), thisNode);

+    return Identifier(reinterpret_cast<const UChar*>(m_impl->characters()),
m_impl->length());

+    return UString(reinterpret_cast<const UChar*>(m_impl->characters()),
m_impl->length());

No typecast needed at all at these call sites now.

There's a small but potentially slightly dangerous difference between
KJS::UChar and the integer type UChar typedef, and that's the behavior of
converting a char into UChar. This will silently behave differently, sign
extending rather than padding with zeroes. Here are the three callsites that
are affected:

>From CStringTranslator::translate:

	for (size_t i = 0; i != length; i++)
	    d[i] = c[i];

This needs a typecast to "unsigned char" on the right side of the assignment
statement. Unless we guarantee we only use this on ASCII strings.

>From SourceStream& SourceStream::operator<<(char c):

    UChar ch(c);

This also needs a typecast to "unsigned char". It's probably a little less
dangerous than the above because we probably use this operator only with ASCII
characters, but I'm not sure.

>From UString::append(const char*), in 3 different places:

	for (int i = 0; i < tSize; ++i)
	    d[thisSize + i] = t[i];

Same issue, same explanation -- safe if only used on ASCII, or we can add a
cast to preserve the old behavior.

I'm going to say review- because of this issue; otherwise good patch.