<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:msaboff@apple.com" title="Michael Saboff <msaboff@apple.com>"> <span class="fn">Michael Saboff</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Strict Equality on objects should only check that one of the two sides is an object."
href="https://bugs.webkit.org/show_bug.cgi?id=145992">bug 145992</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #254990 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Strict Equality on objects should only check that one of the two sides is an object."
href="https://bugs.webkit.org/show_bug.cgi?id=145992#c15">Comment # 15</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Strict Equality on objects should only check that one of the two sides is an object."
href="https://bugs.webkit.org/show_bug.cgi?id=145992">bug 145992</a>
from <span class="vcard"><a class="email" href="mailto:msaboff@apple.com" title="Michael Saboff <msaboff@apple.com>"> <span class="fn">Michael Saboff</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=254990&action=diff" name="attach_254990" title="Patch Expected File">attachment 254990</a> <a href="attachment.cgi?id=254990&action=edit" title="Patch Expected File">[details]</a></span>
Patch Expected File
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=254990&action=review">https://bugs.webkit.org/attachment.cgi?id=254990&action=review</a>
Almost there.
<span class="quote">> LayoutTests/ChangeLog:7
> +</span >
Explain that you added this test for the new optimization.
<span class="quote">> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:5
> + return 0;</span >
It is clearer to return true or false.
<span class="quote">> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:7
> + return 1;</span >
It is clearer to return true or false.
<span class="quote">> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:13
> +for (var i = 1; i < 1000; i++) {</span >
This executes 999 times. Start with i = 0.
<span class="quote">> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:14
> + shouldBe("f(test, test)", "0");</span >
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");
<span class="quote">> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:25
> + for (var i = 1; i < 1000; i++) {
> + shouldBe("f(test, test)", "0");
> + }</span >
Same comments as above.
<span class="quote">> Source/JavaScriptCore/ChangeLog:9
> + to hoist type checks out of a loop we can be cleverer about how we choose</span >
Is cleverer a word?
<span class="quote">> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1175
> + BasicBlock* tmp = taken;
> + taken = notTaken;
> + notTaken = tmp;</span >
Use std::swap()
<span class="quote">> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3950
> + ASSERT(false);</span >
Is this debug code? It needs to be removed.
<span class="quote">> 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);</span >
You could replace this with:
m_jit.compare32(JITCompiler::Equal, op1GPR, op2GPR, resultPayloadGPR);
<span class="quote">> 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);</span >
You can replace this with:
m_jit.compare64(JITCompiler::Equal, op1GPR, op2GPR, resultGPR);
m_jit.or32(TrustedImm32(ValueFalse), resultGPR);</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>