[webkit-reviews] review denied: [Bug 69913] Use realloc() to expand/shrink StringBuilder buffer : [Attachment 110654] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Xianzhu Wang
<wangxianzhu at chromium.org>'s request for review:
Bug 69913: Use realloc() to expand/shrink StringBuilder buffer
https://bugs.webkit.org/show_bug.cgi?id=69913

Attachment 110654: patch
https://bugs.webkit.org/attachment.cgi?id=110654&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=110654&action=review


review- because of the EWS build failures

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:110
> +    // otherwise fallback to "allocate and copy" method.

The verb is “fall back”. The word “fallback” is a noun. This should say “fall
back”.

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:93
> +PassRefPtr<StringImpl> StringImpl::reallocate(PassRefPtr<StringImpl> impl,
unsigned length, UChar*& data)

I think “impl” is a strange name for this argument. You could just as easily
call it “string”. I’d probably call it oldString or existingString or
originalString.

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:112
> +    PassRefPtr<StringImpl> result = adoptRef(new (string)
StringImpl(length));
> +    return result;

The style bot is right to complain about the use of PassRefPtr. The best way to
fix it here is to just call it right in the return statement.

>> Source/JavaScriptCore/wtf/text/StringImpl.h:187
>> +	static PassRefPtr<StringImpl> reallocate(PassRefPtr<StringImpl> impl,
unsigned length, UChar*& data);
> 
> The parameter name "impl" adds no information, so it should be removed. 
[readability/parameter_name] [5]

I agree with the style bot on this.