[webkit-reviews] review granted: [Bug 200614] Replace multiparameter overloads of append() in StringBuilder as a first step toward standardizinging on the flexibleAppend() implementation : [Attachment 376025] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 200614: Replace multiparameter overloads of append() in StringBuilder as a
first step toward standardizinging on the flexibleAppend() implementation
https://bugs.webkit.org/show_bug.cgi?id=200614

Attachment 376025: Patch

https://bugs.webkit.org/attachment.cgi?id=376025&action=review




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 376025
  --> https://bugs.webkit.org/attachment.cgi?id=376025
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376025&action=review

> Source/JavaScriptCore/ChangeLog:9
> +	   Renames StringBuilder::append(const LChar*, unsigned),
StringBuilder::append(const UChar*, unsigned) and 
> +	   StringBuilder::append(const char*, unsigned) to
StringBuilder::appendCharacters(...).

Wow, I was taking a cut at this myself this weekend and I used the *same* names
for the same functions!

> Source/WTF/ChangeLog:16
> +	   * wtf/HexNumber.h:
> +	   (WTF::appendUnsignedAsHexFixedSize):
> +	   Add overload that explicitly takes a StringBuilder to work around
rename from append to appendCharacters.

I took a different approach with this one. The caller that was using this with
a Vector could use a StringBuilder instead, I think. Something we can mess with
later.

> Source/WTF/wtf/HexNumber.h:24
> +#include <wtf/text/StringBuilder.h>

I think we could just do a forward declaration instead of including the entire
header.

> Source/WTF/wtf/text/StringBuilder.h:471
> +    static void flush(LChar* characters, unsigned length, StringBuilder*
stringBuilder) { stringBuilder->appendCharacters(characters, length); }

In my patch I had changed the first argument type to const LChar*.

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:237
> -	   builder.append(name, strlen(name));
> +	   builder.appendCharacters(name, strlen(name));

This can just be append(name) instead since that's what the "const char*"
version does. I suspect it could also use flexibleAppend to do all four appends
on one line.

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:239
> +	   builder.appendCharacters(value, strlen(value));

Ditto.