...
Created attachment 338542 [details] WIP - Patch It starts
Attachment 338542 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:847: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:851: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:852: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:877: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:879: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:908: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:918: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:919: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:924: One space before end of line comments [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:977: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 10 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 338542 [details] WIP - Patch Attachment 338542 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/7400116 New failing tests: stress/big-int-equals-basic.js.big-int-enabled
Created attachment 338556 [details] Patch
Attachment 338556 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:923: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:924: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 338556 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338556&action=review > Source/JavaScriptCore/runtime/JSBigInt.cpp:857 > + return (this->length() == 1) && (this->sign() == (value < 0)) && (this->digit(0) == static_cast<Digit>(std::abs(static_cast<int64_t>(value)))); This works for INT32_MIN, right? > Source/JavaScriptCore/runtime/JSBigInt.cpp:874 > + if (std::isnan(y)) > + return ComparisonResult::Undefined; > + > + if (y == std::numeric_limits<double>::infinity()) > + return ComparisonResult::LessThan; > + > + if (y == -std::numeric_limits<double>::infinity()) > + return ComparisonResult::GreaterThan; I think there is a way to check if it's any of these three in a single branch, check out some of the JIT code I emit here: https://bugs.webkit.org/attachment.cgi?id=334549&action=review We should see if the compiler is smart enough to make this a single branch followed by individual compares. If not, maybe the C++ stdlib has some function that does similarly. Since you're grabbing the raw exponent below, you may as well make it a single branch. > Source/JavaScriptCore/runtime/JSCJSValueInlines.h:984 > + if (v1.isBigInt() && s2) { == for bigint really does type conversions? That's really unfortunate...
Comment on attachment 338556 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338556&action=review Thank you very much for the review. >> Source/JavaScriptCore/runtime/JSBigInt.cpp:857 >> + return (this->length() == 1) && (this->sign() == (value < 0)) && (this->digit(0) == static_cast<Digit>(std::abs(static_cast<int64_t>(value)))); > > This works for INT32_MIN, right? I think so. I'm going to add a test case for that. >> Source/JavaScriptCore/runtime/JSBigInt.cpp:874 >> + return ComparisonResult::GreaterThan; > > I think there is a way to check if it's any of these three in a single branch, check out some of the JIT code I emit here: > https://bugs.webkit.org/attachment.cgi?id=334549&action=review > > We should see if the compiler is smart enough to make this a single branch followed by individual compares. If not, maybe the C++ stdlib has some function that does similarly. > > Since you're grabbing the raw exponent below, you may as well make it a single branch. Cool. I'm going to update that. >> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:984 >> + if (v1.isBigInt() && s2) { > > == for bigint really does type conversions? That's really unfortunate... Yes. Also relational operations "<".
Created attachment 339379 [details] Patch
Attachment 339379 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:923: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:924: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 339647 [details] Patch
Attachment 339647 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:922: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:923: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Ping Review
Comment on attachment 339647 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339647&action=review r=me > Source/JavaScriptCore/runtime/JSBigInt.cpp:867 > + uint64_t doubleBits = *(reinterpret_cast<uint64_t*>(&y)); Let's do `uint64_t doubleBits = bitwise_cast<uint64_t>(y);`. > Source/JavaScriptCore/runtime/JSBigInt.cpp:886 > + if (!y) > + return x->isZero() ? ComparisonResult::Equal : ComparisonResult::GreaterThan; Adding `ASSERT(!xSign)` inside this `if` helps readers.
Comment on attachment 339647 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339647&action=review Thx for the review! >> Source/JavaScriptCore/runtime/JSBigInt.cpp:867 >> + uint64_t doubleBits = *(reinterpret_cast<uint64_t*>(&y)); > > Let's do `uint64_t doubleBits = bitwise_cast<uint64_t>(y);`. Good to know that! I'm just worried about the memcpy of bitwise_cast, but I think it is ok. >> Source/JavaScriptCore/runtime/JSBigInt.cpp:886 >> + return x->isZero() ? ComparisonResult::Equal : ComparisonResult::GreaterThan; > > Adding `ASSERT(!xSign)` inside this `if` helps readers. Done.
Created attachment 340066 [details] Patch for landing
Attachment 340066 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:905: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:906: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 339647 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339647&action=review >>> Source/JavaScriptCore/runtime/JSBigInt.cpp:867 >>> + uint64_t doubleBits = *(reinterpret_cast<uint64_t*>(&y)); >> >> Let's do `uint64_t doubleBits = bitwise_cast<uint64_t>(y);`. > > Good to know that! I'm just worried about the memcpy of bitwise_cast, but I think it is ok. Compilers have optimizations for memcpy, so it produces simple mov related instructions inlinely.
Comment on attachment 340066 [details] Patch for landing Clearing flags on attachment: 340066 Committed r231629: <https://trac.webkit.org/changeset/231629>
<rdar://problem/40201833>