[Webkit-unassigned] [Bug 145992] Strict Equality on objects should only check that one of the two sides is an object.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=145992

Mark Lam <mark.lam at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #255268|review?                     |review-
              Flags|                            |

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

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

> LayoutTests/ChangeLog:9
> +        with document.all.

I think it’d be useful to add a comment about document.all masquerading as undefined.

> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:9
> +var test = {};

nit: Normally, when we say test, we expect some code that runs the test.  I think you mean a testObj or something like that.

> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:14
> +for (var i = 0; i < 1000; i++) {
> +    if (!f(test,test))
> +        testFailed("f(test,test) should have been true but got false");
> +}

Please add a comment above this to indicate that you’re warming up the DFG function here before the "masquerades as undefined” watchpoint fires.

> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:24
> +    for (var i = 0; i < 1000; i++) {
> +        if (!f(test,test))
> +            testFailed("f(test,test) should have been true but got false");
> +    }

Please add a comment above this to indicate that you’re warming up the DFG function here after the "masquerades as undefined” watchpoint fires.

> Source/JavaScriptCore/ChangeLog:8
> +        This patch makes it so as long as we know that at least one side of a strict
> +        equality check is an speculated to be an object we will only type check
> +        that side. Equality is then determined by a pointer comparision between the

typo?  I think you meant, “is speculated to be”.  Also it’d be clearer if you replace “object we” with “object, then we”.

> Source/JavaScriptCore/ChangeLog:9
> +        two values (although in the 32-bit case we must also check the other side

typo: must also check *that*

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1234
> +    MacroAssembler::Jump op2NotCellJump = m_jit.branchIfNotCell(op2.jsValueRegs());

Per our offline discussion, this is incorrect if (taken == nextBlock()).  Let’s not do the swap thing.  Let’s just have 2 different cases.

> Source/JavaScriptCore/jsc.cpp:884
> +    JSValue cell = exec->argument(0);

nit: I think it’s better to call this “value” instead of “cell” since we don’t know if it’s actually a cell.

> Source/JavaScriptCore/jsc.cpp:888
> +    int64_t asNumber = reinterpret_cast<int64_t>(cell.asCell());

It’s better to use a uint64_t here (though we don’t expect the high bit of the cell pointer to be set in practice).

> Source/JavaScriptCore/tests/stress/equality-type-checking.js:18
> +function dfgify(obj, addr) {
> +
> +
> +}

Please delete.

> Source/JavaScriptCore/tests/stress/equality-type-checking.js:22
> +

Please add a comment to explain that you are testing that, on 32-bit builds, strict equality has not neglected to compare the tags as well.  I think this detail is not clear from just reading this code.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150623/1209af87/attachment.html>