Bug 213929 – Convert DateComponents to use StringParsingBuffer

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Convert DateComponents to use StringParsingBuffer

[Sam Weinig]

Created attachment 403465 [details]
Patch

[Sam Weinig]

Created attachment 405583 [details]
Patch

[Sam Weinig]

Created attachment 405591 [details]
Patch

[Darin Adler]

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

[Sam Weinig]

(In reply to Darin Adler from comment #4)
> Comment on attachment 405591 [details]
> 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.

Yeah. Fixed.

> 
> > 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?

I'll see, but so far, unless we convert to using function objects rather than function pointers, we can't deduce CharacterType.

> 
> > 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.

I think this whole maximum number concept can be removed eventually.

> 
> > 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.

Ok. Changed.

> 
> > Source/WebCore/platform/DateComponents.cpp:465
> > +    StringParsingBuffer<CharacterType> temporaryBuffer = buffer;
> 
> Maybe just use auto here?

Fixed.

> 
> > 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.

It's right below. 

                    // Only after successfully parsing and validating 'millisecond', do we increment the real buffer.
                    buffer += 1 + digitsLength;


> 
> > 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.

Nice.

[Sam Weinig]

(In reply to Sam Weinig from comment #5)
> (In reply to Darin Adler from comment #4)
> > Comment on attachment 405591 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=405591&action=review
> 
> > 
> > > 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?
> 
> I'll see, but so far, unless we convert to using function objects rather
> than function pointers, we can't deduce CharacterType.
> 

I guess an alternative option would be to just stop trying to deduce CharacterType and have two overloads of skipWhile, one for LChar, one for UChar:

template<bool characterPredicate(LChar)> void skipWhile(const LChar*& position, const LChar* end)
{
    while (position < end && characterPredicate(*position))
        ++position;
}

template<bool characterPredicate(UChar)> void skipWhile(const UChar*& position, const UChar* end)
{
    while (position < end && characterPredicate(*position))
        ++position;
}

[Sam Weinig]

(In reply to Sam Weinig from comment #6)
> (In reply to Sam Weinig from comment #5)
> > (In reply to Darin Adler from comment #4)
> > > Comment on attachment 405591 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=405591&action=review
> > 
> > > 
> > > > 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?
> > 
> > I'll see, but so far, unless we convert to using function objects rather
> > than function pointers, we can't deduce CharacterType.
> > 
> 
> I guess an alternative option would be to just stop trying to deduce
> CharacterType and have two overloads of skipWhile, one for LChar, one for
> UChar:
> 
> template<bool characterPredicate(LChar)> void skipWhile(const LChar*&
> position, const LChar* end)
> {
>     while (position < end && characterPredicate(*position))
>         ++position;
> }
> 
> template<bool characterPredicate(UChar)> void skipWhile(const UChar*&
> position, const UChar* end)
> {
>     while (position < end && characterPredicate(*position))
>         ++position;
> }

This is untested, might not compile.

[Sam Weinig]

Created attachment 405619 [details]
Patch

[Sam Weinig]

Created attachment 405645 [details]
Patch

[Sam Weinig]

Created attachment 405647 [details]
Patch

[EWS]

Committed r265126: <https://trac.webkit.org/changeset/265126>

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

[Radar WebKit Bug Importer]

<rdar://problem/66359344>