[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 bugzilla-daemon at webkit.org
Thu May 14 02:49:48 PDT 2009

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

Attachment 30312: Fix

------- 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
> +{
> +    if (!characters || !length)
> +	   return empty();
> +
> +    UChar *data;

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

> -    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.

More information about the webkit-reviews mailing list