[webkit-reviews] review granted: [Bug 31330] Expose dtoa() for ECMA-262 : [Attachment 43340] Proposed patch (rev.4)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 31330: Expose dtoa() for ECMA-262
https://bugs.webkit.org/show_bug.cgi?id=31330

Attachment 43340: Proposed patch (rev.4)
https://bugs.webkit.org/attachment.cgi?id=43340&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    UChar* d;
> +    if (!allocChars(length).getValue(d))
> +	   return &UString::Rep::null();
> +    else {
> +	   for (unsigned i = 0; i < length; i++)
> +	       d[i] = static_cast<unsigned char>(c[i]); // use unsigned char to
zero-extend instead of sign-extend
> +	   return UString::Rep::create(d, static_cast<int>(length));
> +    }
> +
> +}

Normally we do early return, not else after return. There's also an extra blank
line here.

Is the static_cast<int> really needed?

I suggest marking this createRep function, which is used in only one place,
inline.

>	   UString();
> +	   // Constructor for null-terminated ASCII string.
>	   UString(const char*);
> +	   // Constructor for non-null-terminated ASCII string.
> +	   UString(const char*, unsigned length);
>	   UString(const UChar*, int length);
>	   UString(UChar*, int length, bool copy);

Given that the other constructors take int for length, why did you chose
unsigned? If this was a new class, I would probably use size_t or perhaps
unsigned, but typically we just keep new consistent with old, locally.

> +    // dtoa() for ECMA-262 'ToString Applied to the Number Type.'
> +    // The *resultLength will have the length of the resultant string in
bufer.
> +    // The resultant string isn't terminated by 0.
> +    void doubleToStringInJavaScriptFormat(double, DtoaBuffer buffer,
unsigned* resultLength);
>  } // namespace WTF

Probably would be slightly nicer to have a blank line before the closing brace.
Could omit the name "buffer" since the type name already says that.

r=me