[webkit-reviews] review granted: [Bug 94562] Store CString data in the CStringBuffer to avoid the double indirection : [Attachment 159738] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 94562: Store CString data in the CStringBuffer to avoid the double
indirection
https://bugs.webkit.org/show_bug.cgi?id=94562

Attachment 159738: Patch
https://bugs.webkit.org/attachment.cgi?id=159738&action=review

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


> Source/WTF/wtf/text/CString.cpp:37
> +    if (length > (numeric_limits<size_t>::max() - sizeof(CStringBuffer) /
sizeof(char)))
> +	   CRASH();

sizeof(char) is defined as 1 by the C and C++ language. It's usually considered
better style to just omit it.

> Source/WTF/wtf/text/CString.cpp:41
> +    size_t size = sizeof(CStringBuffer) + length * sizeof(char);

Same comment about sizeof(char).

> Source/WTF/wtf/text/CString.cpp:57
> +    if (!str)
> +	   return;

I probably would have put an ASSERT(!length) in there too.

> Source/WTF/wtf/text/CString.h:35
> +// CStringBuffer is the ref-counted storage class for the characters of
CStringBuffer.

Should be "for the characters in a CString", not "for the characters of
CStringBuffer".

> Source/WTF/wtf/text/CString.h:36
> +// The data is implicitely allocated 1 character longer than length(), as it
is zero-terminated.

the word implicitly does not have the letter “e”

> Source/WTF/wtf/text/CString.h:49
> +    CStringBuffer(size_t length)
> +	   : m_length(length)
> +    { }

This is not quite the usual style. Normally we put everything on one line, or
everything on different lines. It's unusual to have { } on one line with
everything else on separate lines.

I also suggest marking this constructor explicit.

> Source/WTF/wtf/text/CString.h:52
> +    size_t m_length;

I think this should be defined const.