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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=25779


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


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