Bug 213588

While working on https://bugs.webkit.org/show_bug.cgi?id=213460, it became clear having a typed string type that worked with our parsing idioms would be a nice code improvement. Breaking off that work into this bug.

Created attachment 402746 [details]
Patch

Comment on attachment 402746 [details]
Patch

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

I would prefer if there was at least one use going in with the class.

> Source/WTF/wtf/text/StringParsingBuffer.h:42
> +    constexpr StringParsingBuffer()
> +    {
> +    }

Maybe = default for this one?

> Source/WTF/wtf/text/StringParsingBuffer.h:48
> +    }

Maybe: ASSERT(characters || !length)

> Source/WTF/wtf/text/StringParsingBuffer.h:54
> +    }

ASSERT(!characters == !end) so we don’t pass a null by accident.

> Source/WTF/wtf/text/StringParsingBuffer.h:57
> +    constexpr const CharacterType* position() const { return m_position; }
> +    constexpr const CharacterType* end() const { return m_end; }

Can these use auto instead of const CharacterType*?

> Source/WTF/wtf/text/StringParsingBuffer.h:67
> +        ASSERT(m_position + i < m_end);

Also ASSERT(m_position < m_end) to be overflow-proof?

> Source/WTF/wtf/text/StringParsingBuffer.h:73
> +        return *m_position;

ASSERT(m_position < m_end);

> Source/WTF/wtf/text/StringParsingBuffer.h:78
> +        ++m_position;

ASSERT(m_position < m_end);

> Source/WTF/wtf/text/StringParsingBuffer.h:83
> +        m_position += places;

ASSERT(m_position <= m_end);
ASSERT(m_position + places <= m_end);

> Source/WTF/wtf/text/StringParsingBuffer.h:95
> +        ++(*this);

No need for the parentheses.

Created attachment 402803 [details]
Patch

Comment on attachment 402803 [details]
Patch

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

> Source/WTF/wtf/text/StringParsingBuffer.h:54
> +        ASSERT(m_end >= m_position);
> +        ASSERT(!characters == !end);

Maybe also assert that the length fits in unsigned?

> Source/WTF/wtf/text/StringParsingBuffer.h:69
> +        ASSERT(m_position + i < m_end);

On reflection, because of overflow I think should be written like this:

    ASSERT(i < lengthRemaining());

> Source/WTF/wtf/text/StringParsingBuffer.h:75
> +        ASSERT(m_position < m_end);

New, better idea:

    ASSERT(hasCharactersRemaining());

> Source/WTF/wtf/text/StringParsingBuffer.h:81
> +        ASSERT(m_position < m_end);

Ditto.

> Source/WTF/wtf/text/StringParsingBuffer.h:85
> +    constexpr void advanceBy(unsigned places)

Wonder if this should be size_t instead of unsigned.

> Source/WTF/wtf/text/StringParsingBuffer.h:87
> +        ASSERT(m_position <= m_end);

I guess it was silly that I suggested this one. Obviously, this is the class's invariant, so we could assert this anywhere.

> Source/WTF/wtf/text/StringParsingBuffer.h:88
> +        ASSERT(m_position + places <= m_end);

On reflection, because of overflow I think should be written like this:

    ASSERT(places <= lengthRemaining());

Committed r263529: <https://trac.webkit.org/changeset/263529>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402803 [details].

Hope you don’t mind my after-commit comments!

<rdar://problem/64781816>

(In reply to Darin Adler from comment #8)
> Hope you don’t mind my after-commit comments!

Not in the slightest.

<rdar://problem/64781954>