Bug 188378

Created attachment 346904 [details]
Patch

Attachment 346904 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 346905 [details]
Patch

Created attachment 346906 [details]
Patch

Comment on attachment 346906 [details]
Patch

Attachment 346906 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/8819741

New failing tests:
ChakraCore.yaml/ChakraCore/test/Date/dateutc.js.default
apiTests

Created attachment 346908 [details]
Patch

Comment on attachment 346908 [details]
Patch

r=me. Does this fix any test262 tests?

(In reply to Keith Miller from comment #7)
> Comment on attachment 346908 [details]
> Patch
> 
> r=me. Does this fix any test262 tests?

test/built-ins/Date/UTC/return-value.js has passed! I've added it :D

Committed r234763: <https://trac.webkit.org/changeset/234763>

<rdar://problem/43147473>

Comment on attachment 346908 [details]
Patch

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

> Source/JavaScriptCore/runtime/DateConstructor.cpp:94
> +    double doubleArguments[7] {
> +        0, 0, 1, 0, 0, 0, 0
> +    };

Reads nicely on a single line.

> Source/JavaScriptCore/runtime/DateConstructor.cpp:95
> +    unsigned numberOfUsedArguments = std::max(std::min<unsigned>(7U, args.size()), 1U);

I like to write clamping expressions in sequence like this:

    unsigned numberOfUsedArguments = std::max(1U, std::min<unsigned>(args.size(), 7U));

> Source/JavaScriptCore/runtime/DateConstructor.cpp:101
> +        if (!std::isfinite(doubleArguments[i]) || (doubleArguments[i] > INT_MAX) || (doubleArguments[i] < INT_MIN))

This can be done more efficiently without an explicit isfinite check:

    if (!(doubleArguments[i] >= std::numeric_limits<int>::min() && doubleArguments[i] <= std::numeric_limits<int>::max()))

By doing the checks in this fashion, NaN and both infinities are handled naturally without any explicit check.

> Source/JavaScriptCore/runtime/DateConstructor.cpp:106
>      int year = JSC::toInt32(doubleArguments[0]);

Not new to this patch: I don’t think it is correct to call toInt32 after checking if the value is in range.

I believe this means the range check doesn’t take the rounding done by toInt32 into account and I suspect we will have incorrect results for values right on the edge of the acceptable range. For example, if the value is the maximum integer + 0.1 or the minimum integer - 0.1 it will be treated as NaN, but should not be. Someone should add test cases to cover these edges and then change this to use the correct idiom.

I also suspect that there is a more efficient way to do the JSC::toInt32 operation when we know we don’t want to convert any values outside the 32-bit integer range. For example, maybe std::lround already does the right thing efficiently for that case?

Being more efficient here is not critical, but it would be nice to handle the edges 100% correctly.