[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

Michael Saboff <msaboff at apple.com> changed:

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

--- Comment #15 from Michael Saboff <msaboff at apple.com> ---
Comment on attachment 254990
  --> https://bugs.webkit.org/attachment.cgi?id=254990
Patch Expected File

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

Almost there.

> LayoutTests/ChangeLog:7
> +

Explain that you added this test for the new optimization.

> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:5
> +        return 0;

It is clearer to return true or false.

> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:7
> +    return 1;

It is clearer to return true or false.

> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:13
> +for (var i = 1; i < 1000; i++) {

This executes 999 times.  Start with i = 0.

> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:14
> +    shouldBe("f(test, test)", "0");

Construct this so that you keep track of the the result and provide one shouldBeXXX() or test{Passed,Failed} at the end of the loop.  For example, as long as f(test, test) == 0, you continue the loop.

var result = true;
for (var i = 0; i < 1000; ++i) {
    if (f(test, test) != 0) {
        result = false;
        break;
    }
}

if (result)
    testPassed("f(test, test) compared correctly");
else
    testFailed("f(test, test) did not compare correctly");

> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:25
> +    for (var i = 1; i < 1000; i++) {
> +        shouldBe("f(test, test)", "0");
> +    }

Same comments as above.

> Source/JavaScriptCore/ChangeLog:9
> +        to hoist type checks out of a loop we can be cleverer about how we choose

Is cleverer a word?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1175
> +        BasicBlock* tmp = taken;
> +        taken = notTaken;
> +        notTaken = tmp;

Use std::swap()

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3950
> +            ASSERT(false);

Is this debug code?  It needs to be removed.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1206
> +    MacroAssembler::Jump falseCase = m_jit.branchPtr(MacroAssembler::NotEqual, op1GPR, op2GPR);
> +    m_jit.move(TrustedImm32(1), resultPayloadGPR);
> +    MacroAssembler::Jump done = m_jit.jump();
> +    falseCase.link(&m_jit);
> +    m_jit.move(TrustedImm32(0), resultPayloadGPR);
> +    done.link(&m_jit);

You could replace this with:
    m_jit.compare32(JITCompiler::Equal, op1GPR, op2GPR, resultPayloadGPR);

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1319
> +    MacroAssembler::Jump falseCase = m_jit.branch64(MacroAssembler::NotEqual, op1GPR, op2GPR);
> +    m_jit.move(TrustedImm32(ValueTrue), resultGPR);
> +    MacroAssembler::Jump done = m_jit.jump();
> +    falseCase.link(&m_jit);
> +    m_jit.move(TrustedImm32(ValueFalse), resultGPR);
> +    done.link(&m_jit);

You can replace this with:
    m_jit.compare64(JITCompiler::Equal, op1GPR, op2GPR, resultGPR);
    m_jit.or32(TrustedImm32(ValueFalse), resultGPR);

-- 
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/20150617/8f276e32/attachment-0001.html>