Add a new templated string type to help write more idiomatic parsing code
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 402709 [details] Patch
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>