[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 #31741|review?                     |review-
               Flag|                            |




------- Comment #14 from darin at apple.com  2009-06-23 17:02 PDT -------
(From update of attachment 31741)
Looks better. Thanks again for working on it.

> Index: JavaScriptCore/ChangeLog
> ===================================================================
> --- JavaScriptCore/ChangeLog	(revision 45008)
> +++ JavaScriptCore/ChangeLog	(working copy)
> @@ -1,3 +1,14 @@
> +2009-06-23  Ioseb Dzmanashvili  <ioseb.dzmanashvili at gmail.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * runtime/StringPrototype.cpp:
> +        (JSC::isWhiteSpace):
> +        (JSC::trimString):
> +        (JSC::stringProtoFuncTrim):
> +        (JSC::stringProtoFuncTrimLeft):
> +        (JSC::stringProtoFuncTrimRight):

You need to have a change log entry here, with comments explaining what you
did, and quoting the URL and title of the bug. Look at other ChangeLog entries
to see examples of how to do it. Ideallyou you have a separate comment for each
function.

> +bool isWhiteSpace(int c) 
> +{	
> +    switch (c) {
> +        case 0x20:
> +        case 0xa0:
> +        case 0x1680:
> +        case 0x180e:
> +        case 0x2028:
> +        case 0x2029:
> +        case 0x202f:
> +        case 0x205f:
> +        case 0x3000:
> +            return true;
> +        default:
> +            return c >= 0x09 && c <= 0x0d || c >= 0x2000 && c <= 0x200b || isSeparatorSpace(c);
> +	}
> +}

If this is used only within the file it should be marked "static" to give it
internal linkage.

I don't understand what's going on with this function. It has the name
"isWhiteSpace" which doesn't seem specific enough. Is there any difference
between this and the isStrWhiteSpace function in JSGlobalObjectFunctions.h? Can
you just use that function instead of adding your own?

> +    if(trimMode == TrimRight || trimMode == TrimBothLeftAndRight) {

Need a space here as I mentioned before.

> +    return jsSubstring(exec, s, static_cast<unsigned>(start), static_cast<unsigned>(end - start));

Please remove these unneeded static_cast.

review- because of the isWhiteSpace issue and lack of ChangeLog contents


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