Summary: | Replace multiparameter overloads of append() in StringBuilder as a first step toward standardizinging on the flexibleAppend() implementation | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||
Component: | New Bugs | Assignee: | Sam Weinig <sam> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | andersca, cdumez, commit-queue, darin, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Sam Weinig
2019-08-10 11:07:22 PDT
Created attachment 376019 [details]
Patch
Created attachment 376024 [details]
Patch
Created attachment 376025 [details]
Patch
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. 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. StringBuilder::append, I mean 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 Committed r248552: <https://trac.webkit.org/changeset/248552> (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. (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. (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 |