[webkit-reviews] review granted: [Bug 69320] Some JSValue cleanup : [Attachment 109661] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Geoffrey Garen <ggaren at apple.com>'s
request for review:
Bug 69320: Some JSValue cleanup
https://bugs.webkit.org/show_bug.cgi?id=69320

Attachment 109661: Patch
https://bugs.webkit.org/attachment.cgi?id=109661&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=109661&action=review


> Source/JavaScriptCore/interpreter/Interpreter.cpp:4052
> +	   else if (scrutinee.isDouble() && scrutinee.asDouble() ==
static_cast<int32_t>(scrutinee.asDouble()))
> +	       vPC +=
codeBlock->immediateSwitchJumpTable(tableIndex).offsetForValue(static_cast<int3
2_t>(scrutinee.asDouble()), defaultOffset);

Do the repeated scrutinee.asDouble() and
static_cast<int32_t>(scrutinee.asDouble()) expressions get optimized as we
would wish? I noticed you kept the local variables in another case like this
elsewhere in the patch.

> Source/JavaScriptCore/runtime/JSValueInlineMethods.h:399
>	   return asValue() == jsBoolean(true);

isTrue() above uses JSValue(JSTrue) while this uses jsBoolean(true). Is there
an advantage to one over the other? If not, I suggest unifying.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:462
> +	   unsigned c = static_cast<unsigned>(x);
> +	   if (c == x && c < 36) {

In the old code path, this would all have been done without ever converting to
a double in the case where the value was an Int32.

But if we are really serious about optimizing radix 36 then I think we should
probably do something less general to get the value of the radix too, maybe
even some inlining.