[webkit-reviews] review granted: [Bug 213588] Add a new templated string type to help write more idiomatic parsing code : [Attachment 402746] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 213588: Add a new templated string type to help write more idiomatic
parsing code
https://bugs.webkit.org/show_bug.cgi?id=213588

Attachment 402746: Patch

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 402746
  --> https://bugs.webkit.org/attachment.cgi?id=402746
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.