Bug 200921 – Variadic StringBuilder::append does not handle upconverting from 8-bit to 16-bit correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Darin Adler]

Variadic StringBuilder::append does not handle upconverting from 8-bit to 16-bit correctly

[Darin Adler]

Created attachment 376771 [details]
Patch

[Saam Barati]

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

[Saam Barati]

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

[Darin Adler]

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.

[Darin Adler]

May be a good name is “extend buffer for appending“?

[WebKit Commit Bot]

Comment on attachment 376771 [details]
Patch

Clearing flags on attachment: 376771

Committed r248903: <https://trac.webkit.org/changeset/248903>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54516677>

[Darin Adler]

Oops looks like that JSC failure is real and this needs to be rolled out. Sorry!

[Darin Adler]

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.

[Saam Barati]

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

[Saam Barati]

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?

[Saam Barati]

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.

[Saam Barati]

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.

[Saam Barati]

landed JSC tests fix in:
https://trac.webkit.org/changeset/248913/webkit

[Saam Barati]

(In reply to Darin Adler from comment #5)
> May be a good name is “extend buffer for appending“?

I like this name more.

[Darin Adler]

Thanks for your help, Saam. I'll follow up on the naming changes.

[Darin Adler]

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