Variadic StringBuilder::append does not handle upconverting from 8-bit to 16-bit correctly
Created attachment 376771 [details] Patch
Comment on attachment 376771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376771&action=review > Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:170 > + builder.append(String("A"), "B", bullseye, ""); Do we want to expect !is8Bit() after here? (Same with other tests.)
Comment on attachment 376771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376771&action=review r=me with a couple comments > Source/WTF/wtf/text/StringBuilder.cpp:275 > +UChar* StringBuilder::appendUninitialized16(CheckedInt32 requiredLength) I’m not a fan of these functions names since they don’t actually append. What does the “uninitialized” refer to here? Maybe we should have a prefix like “bufferFor...”?
Comment on attachment 376771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376771&action=review Thanks for the review and the comments. I’m going to land as-is but might follow up later on the comments. >> Source/WTF/wtf/text/StringBuilder.cpp:275 >> +UChar* StringBuilder::appendUninitialized16(CheckedInt32 requiredLength) > > I’m not a fan of these functions names since they don’t actually append. > > What does the “uninitialized” refer to here? > > Maybe we should have a prefix like “bufferFor...”? The concept of this family of 5 private functions is that they append runs of "uninitialized characters", multiple LChar/UChar that need to be written to. The caller gets a pointer to the newly appended characters and must initialize them. The analogy is to the String::createUninitialized and StringImpl::createUninitialized functions. I am open to renaming the family of functions; note that this patch only touches 2 of the 5. I don’t think these should be named "append buffer for uninitialized", but I do think we could come up with a name that’s an improvement. >> Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:170 >> + builder.append(String("A"), "B", bullseye, ""); > > Do we want to expect !is8Bit() after here? (Same with other tests.) I don’t think that is all that helpful. The final check below will check that the string has the correct contents, and that not be possible if it was still 8-bit. The checks that I did add are necessary to make sure that the test is actually testing what it purports to test.
May be a good name is “extend buffer for appending“?
Comment on attachment 376771 [details] Patch Clearing flags on attachment: 376771 Committed r248903: <https://trac.webkit.org/changeset/248903>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54516677>
Oops looks like that JSC failure is real and this needs to be rolled out. Sorry!
Unless someone (Sam, Saam) knows how to fix it and we can roll forward instead. Sadly I can’t work on it during the day today.
(In reply to Darin Adler from comment #10) > Unless someone (Sam, Saam) knows how to fix it and we can roll forward > instead. Sadly I can’t work on it during the day today. I can take a look. Going to kick off a build now.
Comment on attachment 376771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376771&action=review > Source/WTF/wtf/text/StringBuilder.cpp:-304 > - if (!length || hasOverflowed()) maybe this was not ok to remove?
Comment on attachment 376771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376771&action=review >>> Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:170 >>> + builder.append(String("A"), "B", bullseye, ""); >> >> Do we want to expect !is8Bit() after here? (Same with other tests.) > > I don’t think that is all that helpful. The final check below will check that the string has the correct contents, and that not be possible if it was still 8-bit. The checks that I did add are necessary to make sure that the test is actually testing what it purports to test. I don't think it's critical, but I think it's nice when reading testing code like this that all the expectations are explicitly checked, even if they may be redundant. I like to read testing code which asserts that all the invariants are held explicitly.
Comment on attachment 376771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376771&action=review >> Source/WTF/wtf/text/StringBuilder.cpp:-304 >> - if (!length || hasOverflowed()) > > maybe this was not ok to remove? Yeah, these two "hasOverflowed()" checks were not valid to remove. I haven't dug into why, but I'll revert their removal.
landed JSC tests fix in: https://trac.webkit.org/changeset/248913/webkit
(In reply to Darin Adler from comment #5) > May be a good name is “extend buffer for appending“? I like this name more.
Thanks for your help, Saam. I'll follow up on the naming changes.
(In reply to Saam Barati from comment #13) > I don't think it's critical, but I think it's nice when reading testing code > like this that all the expectations are explicitly checked, even if they may > be redundant. I like to read testing code which asserts that all the > invariants are held explicitly. I’ll consider that, but I’m not convinced it would make the tests better. It would make them brittle to changes that are observable that are not important, and there are many invariants we could assert on the string builder before and after each operation.