[Webkit-unassigned] [Bug 26590] Support for String.trim(), String.trimLeft() and String.trimRight() methods

[bugzilla-daemon at webkit.org]

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


darin at apple.com changed:

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




------- Comment #11 from darin at apple.com  2009-06-23 13:03 PDT -------
(From update of attachment 31724)
Thanks very much for addressing my comments from the early version. I now read
this even more closely and have some new comments.

I have doubts about is the isWhiteSpace function.

There is already an isWhiteSpace function in Lexer, an isStrWhiteSpace function
in JSGlobalObjectFunctions, and WTF::Unicode::isSeparatorSpace.

I suspect that this new isWhiteSpace function is identical to
WTF::Unicode::isSeparatorSpace -- could you check on that?

Also, I'm not sure it's appropriate for the large isWhiteSpace function to be
marked inline, and I suspect that the const int array ends up being re-created
and initialized every time the function is called. None of this matters if we
can just use isSeparatorSpace.

> +        (JSC::):

Please don't leave a line like this in the ChangeLog, even if the
prepare-ChangeLog script puts it there.

> +static inline JSValue trimString(ExecState* exec, JSValue thisValue, TrimMode trimMode) 
> +{		
> +    JSString* sVal = thisValue.toThisJSString(exec);
> +    const UString& s = sVal->value();
> +		
> +    int sSize = s.size();
> +    if (!sSize)
> +        return sVal;
> +		
> +    const UChar* sData = s.data();
> +		
> +    int start = 0;
> +    int end = sSize - 1;
> +		
> +    if (trimMode == TrimLeft || trimMode == TrimBothLeftAndRight) {
> +        while (start < sSize && isWhiteSpace(sData[start])) {
> +            start++;
> +        }
> +    }
> +		
> +    if(trimMode == TrimRight || trimMode == TrimBothLeftAndRight) {
> +        while (end >= start && isWhiteSpace(sData[end])) {
> +            end--;
> +        }
> +    }
> +	
> +    return jsSubstring(exec, s, static_cast<unsigned>(start), static_cast<unsigned>((end - start) + 1));
> +}

I think the logic would be slightly clearer if end pointed past the last
character instead of at the last character. You would have to say "end - 1" in
the place you call isWhiteSpace, but otherwise it would be easier to read the
function.

I suggest using unsigned instead of int for end, start, and sSize. You only
need a signed int because of end being a pointer before the last character, I
think. You could get rid of the type casts, then.

I think sSize could just be named size.

No braces around single line bodies of while. Need a space after if and before
the open parenthesis.

This needs a special case to return sVal when start is 0 and end is sSize. I
think it's common to call trim and not trim anything, and in that case we don't
want to allocate a new JSString cell.

There is no need for the special case for a size of zero. The rest of the
function already does the right thing for that case already, including the
jsSubstring function which won't allocate anything for a zero-length string,
and I don't think an empty string needs extra performance optimization.

> +var successfullyParsed = true;
> \ No newline at end of file

Please add the newline here.

I'd like to see more test cases of strings that don't need to be trimmed,
including characters that might seem to be spaces but don't qualify, such as
control characters. If we can make the test run fast enough it might even be
good to find a way to test many other characters to see they are not trimmed. I
can't think of an efficient way to do it on all 2^21 characters, though, so you
might have to be creative.

review- for now since I'd like you to do at least some of the things I suggest.


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