[webkit-reviews] review denied: [Bug 137165] Support modern for loops over StringView code units and code points : [Attachment 238931] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 137165: Support modern for loops over StringView code units and code points
https://bugs.webkit.org/show_bug.cgi?id=137165

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

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


review- because we don’t want StringView == StringView to do what is coded
here.

> Source/WTF/wtf/text/StringView.h:41
> +    class CodePoints;
> +    class CodeUnits;

I suggest defining these right before they are used instead of at the top of
the class.

> Source/WTF/wtf/text/StringView.h:188
> +    bool operator==(const StringView&) const;
> +    bool operator!=(const StringView&) const;

These operators should be implemented as free functions, not member functions.
If these are member functions, then no type conversions can be done on the left
side of the == and !=, but if they are free functions they can be.

> Source/WTF/wtf/text/StringView.h:247
> +#if !ASSERT_DISABLED
> +	   mutable Mode m_mode;
> +#endif

I think a boolean named:

    bool m_alreadyIncremented;

or something like that, would be clearer than this enum, which I find
mysterious.

> Source/WTF/wtf/text/StringView.h:339
> +inline bool StringView::operator==(const StringView& other) const
> +{
> +    return m_characters == other.m_characters && m_length == other.m_length;

> +}

I don’t think it’s good idea to have a StringView == operator that checks
identity rather than contents of the strings. I think an == operator on two
string views should compare the actual characters, not the pointers. And should
return true if the two views look at two different ranges of memory with the
same characters, even if one is 8-bit and the other 16-bit.

Or we could just not have an == operator, but having one with this meaning
seems like a bad idea.

> Source/WTF/wtf/text/StringView.h:396
> +    return m_stringView == other.m_stringView && m_index == other.m_index;

This should assert that &m_stringView == &other.m_stringView and should not
look at m_stringView at all in non-debug builds.

> Source/WTF/wtf/text/StringView.h:438
> +    return m_stringView == other.m_stringView && m_index == other.m_index;

This should assert that &m_stringView == &other.m_stringView and should not
look at m_stringView at all in non-debug builds.