[webkit-reviews] review granted: [Bug 213929] Convert DateComponents to use StringParsingBuffer : [Attachment 405591] 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 213929: Convert DateComponents to use StringParsingBuffer
https://bugs.webkit.org/show_bug.cgi?id=213929

Attachment 405591: Patch

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




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

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

> Source/WebCore/platform/DateComponents.cpp:44
> +// HTML5 uses ISO-8601 format with year >= 1. Gregorian calendar started in

Continuing to call the HTML specification "HTML5" is quaint.

> Source/WebCore/platform/DateComponents.cpp:112
> +    skipWhile<CharacterType, isASCIIDigit>(buffer);

Is there no way to make this idiom figure out the character type from the
buffer argument?

> Source/WebCore/platform/DateComponents.cpp:120
> +    if (maximumNumberOfDigitsToParse > buffer.lengthRemaining() ||
!maximumNumberOfDigitsToParse)
> +	   return WTF::nullopt;

Subtly awkward naming that caller must *not* pass in a large value for
maximumNumberOfDigitsToParse. Doesn't really feel like it’s a maximum from the
caller’s point of view. The function could clamp instead of just returning.

> Source/WebCore/platform/DateComponents.cpp:142
> +    if (value && (*value < minimumValue || *value > maximumValue))

I would have written:

    if (!(value && *value >= minimumValue && *value <= maximumValue))

Handles null in a different way, but I slightly prefer it. I think one reason
is that this idiom does the right thing with non-finite floating point values.
Also if refactoring to use a function, it’s more logical to have an "is good"
function than an "is bad" function. In fact, I kind of wish the
WTF::String::isNull function had the opposite sense and had some awesome name
for the same reason.

> Source/WebCore/platform/DateComponents.cpp:465
> +    StringParsingBuffer<CharacterType> temporaryBuffer = buffer;

Maybe just use auto here?

> Source/WebCore/platform/DateComponents.cpp:485
> +			   // Regardless of the number of digits, we only ever
parse at most 3. All other
> +			   // digits after that are ignored, but the buffer is
incremented as if they were
> +			   // all parsed.

I don’t see the code that increments the buffer as if they were all parsed.

> Source/WebCore/platform/DateComponents.cpp:659
> +    if (doubleYear < minimumYear || maximumYear < doubleYear)

Same thought about writing this in the !"is good" way instead of the "is bad"
way. And here it’s literally floating point, so reversing it would let us
optimize by removing the std::isfinite check above.

> Source/WebCore/platform/DateComponents.cpp:697
> +	   if (m_year <= minimumYear)

Fascinating that this is "<=". I’m sure we have test coverage.