Bug 46509 – Add operator == for AtomicString and Vector<Uchar>

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Erik Arvidsson]

Add operator == for AtomicString and Vector<Uchar>

[Erik Arvidsson]

Created attachment 68749 [details]
Patch

[Erik Arvidsson]

Darin suggested that I added this function in bug 20709.

[Darin Adler]

Comment on attachment 68749 [details]
Patch

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.

[Erik Arvidsson]

Created attachment 68799 [details]
Patch

[Darin Adler]

Comment on attachment 68799 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68799&action=review

> JavaScriptCore/wtf/text/AtomicString.h:129
> +inline bool operator==(const AtomicString& a, const Vector<UChar>& b) { return a.impl() && equal(a.impl(), b.data(), b.size()); }

Oops! This won’t compile. The equal function this calls is inside AtomicString.cpp and private to the cpp file, not exposed to the header at all.

[Erik Arvidsson]

Comment on attachment 68799 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68799&action=review

>> JavaScriptCore/wtf/text/AtomicString.h:129
>> +inline bool operator==(const AtomicString& a, const Vector<UChar>& b) { return a.impl() && equal(a.impl(), b.data(), b.size()); }
> 
> Oops! This won’t compile. The equal function this calls is inside AtomicString.cpp and private to the cpp file, not exposed to the header at all.

Yeah, I apologize for that patch upload. I'm moving it to the cpp and compiling and testing again.

[Eric Seidel (no email)]

Attachment 68799 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4061109

[Erik Arvidsson]

Created attachment 68937 [details]
Patch

[Darin Adler]

Comment on attachment 68937 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68937&action=review

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

There should be a blank line there.

We could use the names "string" and "vector" instead of "a" and "b" here.

[Erik Arvidsson]

Comment on attachment 68937 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68937&action=review

Thanks for your patience.

>> JavaScriptCore/wtf/text/AtomicString.cpp:158
>> +bool operator==(const AtomicString& a, const Vector<UChar>& b)
> 
> There should be a blank line there.
> 
> We could use the names "string" and "vector" instead of "a" and "b" here.

Done and done.

[Erik Arvidsson]

Committed r68422: <http://trac.webkit.org/changeset/68422>