Bug 201020 - Rename StringBuilder functions to avoid unclear "append uninitialized" terminology
Summary: Rename StringBuilder functions to avoid unclear "append uninitialized" termin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-21 19:09 PDT by Darin Adler
Modified: 2019-08-22 09:51 PDT (History)
9 users (show)

See Also:


Attachments
Patch (9.83 KB, patch)
2019-08-21 19:10 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (9.83 KB, patch)
2019-08-21 20:29 PDT, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2019-08-21 19:09:55 PDT
Rename StringBuilder functions to avoid unclear "append uninitialized" terminology
Comment 1 Darin Adler 2019-08-21 19:10:56 PDT Comment hidden (obsolete)
Comment 2 Darin Adler 2019-08-21 20:29:20 PDT
Created attachment 376972 [details]
Patch
Comment 3 Alex Christensen 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?
Comment 4 Darin Adler 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.
Comment 5 WebKit Commit Bot 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.
Comment 6 WebKit Commit Bot 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>
Comment 7 WebKit Commit Bot 2019-08-22 09:50:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2019-08-22 09:51:20 PDT
<rdar://problem/54602473>