Bug 200614 – Replace multiparameter overloads of append() in StringBuilder as a first step toward standardizinging on the flexibleAppend() implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Replace multiparameter overloads of append() in StringBuilder as a first step toward standardizinging on the flexibleAppend() implementation

[Sam Weinig]

Created attachment 376019 [details]
Patch

[Sam Weinig]

Created attachment 376024 [details]
Patch

[Sam Weinig]

Created attachment 376025 [details]
Patch

[Darin Adler]

Comment on attachment 376025 [details]
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.

[Darin Adler]

I noticed some other differences between the string adapter behavior and overloads of String::append; the big one was append(UChar32) which from an overloading point of view is the same as append(int). So I decided that needs to be renamed, at least temporarily, to appendCharacter.

[Darin Adler]

StringBuilder::append, I mean

[WebKit Commit Bot]

Comment on attachment 376025 [details]
Patch

Rejecting attachment 376025 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 376025, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebKit/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: https://webkit-queues.webkit.org/results/12898538

[Sam Weinig]

Committed r248552: <https://trac.webkit.org/changeset/248552>

[Radar WebKit Bug Importer]

<rdar://problem/54231521>

[Sam Weinig]

(In reply to Darin Adler from comment #4)
> Comment on attachment 376025 [details]
> 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.

Yeah, this might have been better. I was worried since it was a debugging thing that there might be some lldb scripts that depend on it returning a Vector, but I could have just converted to a Vector at the end.

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

No, it needed the #include due to inlining.

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

I will do this in a follow up, but change all the flush() implementations at once. I didn't want to change just this one.

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

Done.

[Radar WebKit Bug Importer]

<rdar://problem/54231610>

[Sam Weinig]

(In reply to Darin Adler from comment #5)
> I noticed some other differences between the string adapter behavior and
> overloads of String::append; the big one was append(UChar32) which from an
> overloading point of view is the same as append(int). So I decided that
> needs to be renamed, at least temporarily, to appendCharacter.

Oh, good catch. I'll make that change. I am curious where in the code we are using UChar32, I guess I will find out soon.

[Sam Weinig]

(In reply to Sam Weinig from comment #12)
> (In reply to Darin Adler from comment #5)
> > I noticed some other differences between the string adapter behavior and
> > overloads of String::append; the big one was append(UChar32) which from an
> > overloading point of view is the same as append(int). So I decided that
> > needs to be renamed, at least temporarily, to appendCharacter.
> 
> Oh, good catch. I'll make that change. I am curious where in the code we are
> using UChar32, I guess I will find out soon.

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