Improve assertions in StringParsingBuffer

[Sam Weinig]

Improve assertions in StringParsingBuffer

[Sam Weinig]

Created attachment 402838 [details]
Patch

[Sam Weinig]

Created attachment 402841 [details]
Patch

[Darin Adler]

Comment on attachment 402841 [details]
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.

[Sam Weinig]

Created attachment 402907 [details]
Patch

[Darin Adler]

Comment on attachment 402907 [details]
Patch

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

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

<= ?

[Sam Weinig]

Created attachment 402956 [details]
Patch

[EWS]

Committed r263620: <https://trac.webkit.org/changeset/263620>

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

[Radar WebKit Bug Importer]

<rdar://problem/64844270>

[Radar WebKit Bug Importer]

<rdar://problem/64844271>