[webkit-reviews] review denied: [Bug 137165] Support modern for loops over StringViews : [Attachment 238754] 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 StringViews
https://bugs.webkit.org/show_bug.cgi?id=137165

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

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


I think this is a pretty good idea.

But what guarantees these objects don’t last longer than the underlying
StringView? It seems really easy to make the mistake of keeping one of these
alive when the StringView is gone. For example, this code would not work:

    for (UChar character : functionReturningStringView().codeUnits())

I think we should consider a different version of these that will work even if
the StringView has been destroyed. Probably the best way to do this is to use a
StringView rather than a const StringView& as a data member of the iterator.

We need to test how codePoints works on invalid sequences; such as when the
last character is the lead and there is no trail, and when a trail appears with
no lead. U16_NEXT handles these cases, but I am not sure that our code handles
them correctly.

I am not sure that

> Source/WTF/ChangeLog:8
> +	   This patch adds two functions, codepoints() and codeunits(), on
StringView.

Code point and code unit are each two word phrases, not single words. Lets not
smash them together into one word. uNless there is a good reason.

> Source/WTF/wtf/text/StringView.h:40
> +class StringViewCodepoints {

I think this should be a member class. So it would be declared inside the
StringView class.

    class StringView {
	... 
	class CodePoints;

Then later, like this:

    class StringView::CodePoints {
	...
    };

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

I think we should supply both == and != operators.

> Source/WTF/wtf/text/StringView.h:57
> +    StringViewCodepoints(const StringView&);

Might want this to be explicit.

> Source/WTF/wtf/text/StringView.h:60
> +private:

Need a blank line before this.

> Source/WTF/wtf/text/StringView.h:240
> +    StringViewCodepoints codepoints() const
> +    {
> +	   return StringViewCodepoints(*this);
> +    }
> +
> +    StringViewCodeunits codeunits() const
> +    {
> +	   return StringViewCodeunits(*this);
> +    }

Please put the bodies of these functions outside the class. I have a patch
coming soon that does this for all StringView member functions.

> Source/WTF/wtf/text/StringView.h:348
> +    if (m_cacheIsValid) {
> +	   m_index = m_nextIndexCache;
> +	   m_cacheIsValid = false;

Would read better with early return.

> Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:158
> +    for (auto codepoint : StringView(builder.toString()).codepoints()) {

I think this code would crash if it was run under a memory debugger; the
StringView won’t stay alive.