[webkit-reviews] review denied: [Bug 25779] Allow Strings to be created with one malloc node with no copying : [Attachment 30312] Fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Dave Moore <davemoore at google.com>'s
request for review:
Bug 25779: Allow Strings to be created with one malloc node with no copying
https://bugs.webkit.org/show_bug.cgi?id=25779

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   * platform\text\PlatformString.h:
> +	   * platform\text\String.cpp:
> +	   * platform\text\StringImpl.cpp:
> +	   * platform\text\StringImpl.h
> +:

The paths here are using the Windows-style \ patch separator.

>      unsigned end = m_length - 1;
> -    
> +
>      // skip white space from start

Your patch here strips all trailing whitespace from this file. Although we have
no formal policy about this, and I believe the git tool does encourage you to
do this, we normally discourage including changes like that in the same
check-in with substantive changes, since it confuses things in both that
check-in and the file history.

We also don't have agreement on whether to do something like this project-wide,
and that's something I'd like to see before taking patches that just remove
trailing whitespace.

There are more lines in your patch from the whitespace change than from the
substantive one.

> +    if (!length) {
> +	   data = NULL;

We use 0 in this project, not NULL. See the WebKit style guide.

> +PassRefPtr<StringImpl> StringImpl::create(const UChar* characters, unsigned
length)
> +{
> +    if (!characters || !length)
> +	   return empty();
> +
> +    UChar *data;

We put the * next to the type -- UChar*, not UChar * -- see the WebKit style
guide.

> -    UChar* data = reinterpret_cast<UChar*>(buffer + sizeof(StringImpl));
> +    UChar *data;

Same here.

I'm slightly concerned that this patch adds a branch or two and a new level of
function call to a common code path. I assume it won't slow anything down
measurably though.

The substance of the patch looks good. Please fix these style issues.