[webkit-reviews] review denied: [Bug 46509] Add operator == for AtomicString and Vector<Uchar> : [Attachment 68749] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Erik Arvidsson <arv at chromium.org>'s
request for review:
Bug 46509: Add operator == for AtomicString and Vector<Uchar>
https://bugs.webkit.org/show_bug.cgi?id=46509

Attachment 68749: Patch
https://bugs.webkit.org/attachment.cgi?id=68749&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=68749&action=review

> JavaScriptCore/wtf/text/AtomicString.cpp:159
> +bool operator==(const AtomicString& a, const Vector<UChar>& b)

Unlike strings, there is no concept of a null vector as distinct from an empty
vector, so there should not be code here checking b.data() for null.

> JavaScriptCore/wtf/text/AtomicString.cpp:161
> +    StringImpl* impl = a.impl();

It doesn’t help readability or efficiency to put this into a local variable.

> JavaScriptCore/wtf/text/AtomicString.cpp:163
> +    if ((!impl || !impl->characters()) && !s)

There’s no need to check null impl->characters().

> JavaScriptCore/wtf/text/AtomicString.cpp:167
> +    return equal(impl, s, b.size());

This whole function can just be:

    return a.impl() && equal(a.impl(), b.data(), b.size());

No need for any other null checks.