[webkit-reviews] review granted: [Bug 213440] Convert DateComponents parsing code to use Optional based return values rather than out-parameters : [Attachment 402425] 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 213440: Convert DateComponents parsing code to use Optional based return
values rather than out-parameters
https://bugs.webkit.org/show_bug.cgi?id=213440

Attachment 402425: Patch

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




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

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

> Source/WebCore/html/BaseDateAndTimeInputType.cpp:81
> +    return !value.isEmpty() && parseToDateComponents(value) == WTF::nullopt;

I think !parse reads better than parse == nullopt. But I suppose others might
argue that the ! one looks like it’s about 0; not for me. (As an aside, I hope
we change our style to allow "== 0" at some point.)

> Source/WebCore/html/BaseDateAndTimeInputType.h:58
> +    virtual Optional<DateComponents> setMillisecondToDateComponents(double)
const = 0;

What does the word "set" refer to in this (already existing, not new to this
patch) function name?

> Source/WebCore/platform/DateComponents.cpp:116
> +    const CharacterType* current = src + parseStart;
> +    const CharacterType* end = current + parseLength;

I just woulda done auto for these.

> Source/WebCore/platform/DateComponents.cpp:201
> +    return createFromString(source, [] (auto& date, const auto* src, auto
length, auto& end) {

Not a big fan of the use of the string "src" here. Also I think just auto src
would do, no need for const auto*.