Bug 201020

Summary: Rename StringBuilder functions to avoid unclear "append uninitialized" terminology
Product: WebKit Reporter: Darin Adler <darin>
Component: Web Template FrameworkAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, benjamin, cdumez, cmarcelo, commit-queue, dbates, ews-watchlist, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Darin Adler
Reported 2019-08-21 19:09:55 PDT
Rename StringBuilder functions to avoid unclear "append uninitialized" terminology
Attachments
Patch (9.83 KB, patch)
2019-08-21 19:10 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
Patch (9.83 KB, patch)
2019-08-21 20:29 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Darin Adler
Comment 1 2019-08-21 19:10:56 PDT Comment hidden (obsolete)
Darin Adler
Comment 2 2019-08-21 20:29:20 PDT
Created attachment 376972 [details] Patch
Alex Christensen
Comment 3 2019-08-22 07:28:46 PDT
Comment on attachment 376972 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376972&action=review > Source/WTF/wtf/text/StringBuilder.cpp:134 > + std::memcpy(m_bufferCharacters16, currentCharacters, static_cast<size_t>(m_length.unsafeGet()) * sizeof(UChar)); // This can't overflow. Why did you remove the other "This can't overflow." comment but not this one?
Darin Adler
Comment 4 2019-08-22 08:55:55 PDT
Comment on attachment 376972 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376972&action=review >> Source/WTF/wtf/text/StringBuilder.cpp:134 >> + std::memcpy(m_bufferCharacters16, currentCharacters, static_cast<size_t>(m_length.unsafeGet()) * sizeof(UChar)); // This can't overflow. > > Why did you remove the other "This can't overflow." comment but not this one? The comment here has a meaning even though it's not as specific and clear as I would like: limits enforced on m_length elsewhere in this class and other WTF::String family classes are presumably the reason this multiplication by 2 can’t overflow, even on a 32-bit platform where size_t is 32-bit. The other comment was different. It was multiplying by sizeof(LChar), which is 1; obviously multiplying by 1 won’t overflow. I removed both the multiplication and the comment because I don’t think we need an explicit typecast and multiplication; then there’s no math at all so no need to say anything about overflow. If we really were worried that sizeof(LChar) == 1 was non-obvious we could have put in a compile time assertion, but I didn’t think that was needed either.
WebKit Commit Bot
Comment 5 2019-08-22 09:49:49 PDT
The commit-queue encountered the following flaky tests while processing attachment 376972 [details]: webgl/2.0.0/conformance/ogles/GL/mat/mat_025_to_032.html bug 201043 (author: justin_fan@apple.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 6 2019-08-22 09:50:34 PDT
Comment on attachment 376972 [details] Patch Clearing flags on attachment: 376972 Committed r249015: <https://trac.webkit.org/changeset/249015>
WebKit Commit Bot
Comment 7 2019-08-22 09:50:35 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 8 2019-08-22 09:51:20 PDT
<rdar://problem/54602473>
Note You need to log in before you can comment on or make changes to this bug.