...
Created attachment 397236 [details] patch
Comment on attachment 397236 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=397236&action=review > Source/JavaScriptCore/ChangeLog:9 > + if the value could be an int32. This patch makes the algorithm precise on I think predictable is a better description here than precise.
Comment on attachment 397236 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=397236&action=review Thanks for improving this. I have a couple nits and questions below: > Source/JavaScriptCore/jsc.cpp:2331 > + return JSValue::encode(jsBoolean(!!jsDynamicCast<JSBigInt*>(globalObject->vm(), callFrame->argument(0)))); Is there a reason for using this weird !!jsDynamicCast<JSBigInt*> instead of JSValue::isHeapBigInt ? > Source/JavaScriptCore/runtime/JSBigInt.cpp:1876 > + auto limitWorks = [&] (double radix, double lengthLimit) { I don't think you need to pass lengthLimit as an argument, it could just be lengthLimitForBigInt32 which is captured. > Source/JavaScriptCore/runtime/JSBigInt.cpp:1906 > + // radix ** lengthLimitForBigInt32 <= INT32_MAX Why is this the opposite from the BigInt32 case? > Source/JavaScriptCore/runtime/JSBigInt.cpp:1908 > + auto limitWorks = [&] (double radix, double lengthLimit) { same comment as before: no need to pass lengthLimit explicitly. > Source/JavaScriptCore/runtime/JSBigInt.cpp:1967 > + int64_t maybeResult = digit.unsafeGet(); maybe an assert here that going from uint64_t to int64_t does not overflow? I see no way that it could, but an assert would be reassuring if we ever modify any of that code again.
Comment on attachment 397236 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=397236&action=review >> Source/JavaScriptCore/ChangeLog:9 >> + if the value could be an int32. This patch makes the algorithm precise on > > I think predictable is a better description here than precise. The old algorithm was also predictable, it's bounds were just worse. >> Source/JavaScriptCore/jsc.cpp:2331 >> + return JSValue::encode(jsBoolean(!!jsDynamicCast<JSBigInt*>(globalObject->vm(), callFrame->argument(0)))); > > Is there a reason for using this weird !!jsDynamicCast<JSBigInt*> instead of JSValue::isHeapBigInt ? nope. I just didn't know it existed. >> Source/JavaScriptCore/runtime/JSBigInt.cpp:1876 >> + auto limitWorks = [&] (double radix, double lengthLimit) { > > I don't think you need to pass lengthLimit as an argument, it could just be lengthLimitForBigInt32 which is captured. ok >> Source/JavaScriptCore/runtime/JSBigInt.cpp:1906 >> + // radix ** lengthLimitForBigInt32 <= INT32_MAX > > Why is this the opposite from the BigInt32 case? Because Digit is 32-bit unsigned here, and I don't want to consider what happens below in static_cast<Digit>(uint64_t value) >> Source/JavaScriptCore/runtime/JSBigInt.cpp:1908 >> + auto limitWorks = [&] (double radix, double lengthLimit) { > > same comment as before: no need to pass lengthLimit explicitly. ok >> Source/JavaScriptCore/runtime/JSBigInt.cpp:1967 >> + int64_t maybeResult = digit.unsafeGet(); > > maybe an assert here that going from uint64_t to int64_t does not overflow? I see no way that it could, but an assert would be reassuring if we ever modify any of that code again. good idea
Created attachment 397242 [details] patch
Comment on attachment 397242 [details] patch r=me
Created attachment 397249 [details] patch for landing
Comment on attachment 397249 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=397249&action=review > Source/JavaScriptCore/jsc.cpp:2331 > + return JSValue::encode(jsBoolean(!!jsDynamicCast<JSBigInt*>(globalObject->vm(), callFrame->argument(0)))); oops, forgot about this. Will fix.
Created attachment 397250 [details] patch for landing
Created attachment 397251 [details] patch for landing
Created attachment 397252 [details] patch for landing
Committed r260550: <https://trac.webkit.org/changeset/260550> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397252 [details].
<rdar://problem/62227321>