| Summary: | Rename StringBuilder functions to avoid unclear "append uninitialized" terminology | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||
| Component: | Web Template Framework | Assignee: | 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
Darin Adler
2019-08-21 19:09:55 PDT
Created attachment 376964 [details]
Patch
Created attachment 376972 [details]
Patch
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? 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. 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. Comment on attachment 376972 [details] Patch Clearing flags on attachment: 376972 Committed r249015: <https://trac.webkit.org/changeset/249015> All reviewed patches have been landed. Closing bug. |