[Webkit-unassigned] [Bug 25779] Allow Strings to be created with one malloc node with no copying

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu May 14 02:49:48 PDT 2009


darin at apple.com changed:

           What    |Removed                     |Added
  Attachment #30312|review?                     |review-
               Flag|                            |

------- Comment #2 from darin at apple.com  2009-05-14 02:49 PDT -------
(From update of attachment 30312)
> +        * 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

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

Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

More information about the webkit-unassigned mailing list