[webkit-reviews] review granted: [Bug 213633] Improve assertions in StringParsingBuffer : [Attachment 402841] 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 213633: Improve assertions in StringParsingBuffer
https://bugs.webkit.org/show_bug.cgi?id=213633

Attachment 402841: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 402841
  --> https://bugs.webkit.org/attachment.cgi?id=402841
Patch

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

> Source/WTF/wtf/text/StringParsingBuffer.h:55
> +	   ASSERT(static_cast<std::ptrdiff_t>(end - characters) <
std::numeric_limits<unsigned>::max());

Surprised that the static_cast on the left side is necessary. Surprised that a
static_cast on the right side isn’t necessary to ensure we don’t get
signed/unsigned comparison warnings.

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

Not sure how I feel about this even though I suggested it. In general if we
pass too large a type it will get chopped off. But if the value is too big for
unsigned the code won’t work anyway. Not sure why this is more important than
taking size_t in operator[]. Could imagine setting this to a super-large
integer type and doing a RELEASE_ASSERT. Maybe we should just be leaving this
unsigned? Issues don’t seem different here than in operator[] or the
constructor.