[webkit-reviews] review granted: [Bug 223783] [JSC] Use new Apple ICU APIs to avoid C++ ICU API usage : [Attachment 424339] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 223783: [JSC] Use new Apple ICU APIs to avoid C++ ICU API usage
https://bugs.webkit.org/show_bug.cgi?id=223783

Attachment 424339: Patch

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




--- Comment #9 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 424339
  --> https://bugs.webkit.org/attachment.cgi?id=424339
Patch

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

r=me.  Please make sure that the mac-AS-debug-wk2 bot failure is not real.

> Source/JavaScriptCore/runtime/JSDateMath.cpp:133
> +    // The above can fail if input date is invalid: NaN etc.
> +    // We can return any values in this case since later we fail when
computing non timezone offset part anyway.
> +    constexpr LocalTimeOffset failed { false, 0 };

"The above" sounds wrong since there's no code that has any effect above.  How
about changing it to "This function"?

This failed return value also seem to contradict the comment above the function
which states that "If this function is called with NaN it returns NaN."  Maybe
remove the comment above the function?