<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:mark.lam@apple.com" title="Mark Lam <mark.lam@apple.com>"> <span class="fn">Mark Lam</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 #255268 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#c29">Comment # 29</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:mark.lam@apple.com" title="Mark Lam <mark.lam@apple.com>"> <span class="fn">Mark Lam</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=255268&action=diff" name="attach_255268" title="Patch">attachment 255268</a> <a href="attachment.cgi?id=255268&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=255268&action=review">https://bugs.webkit.org/attachment.cgi?id=255268&action=review</a>
<span class="quote">> LayoutTests/ChangeLog:9
> + with document.all.</span >
I think it’d be useful to add a comment about document.all masquerading as undefined.
<span class="quote">> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:9
> +var test = {};</span >
nit: Normally, when we say test, we expect some code that runs the test. I think you mean a testObj or something like that.
<span class="quote">> 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");
> +}</span >
Please add a comment above this to indicate that you’re warming up the DFG function here before the "masquerades as undefined” watchpoint fires.
<span class="quote">> 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");
> + }</span >
Please add a comment above this to indicate that you’re warming up the DFG function here after the "masquerades as undefined” watchpoint fires.
<span class="quote">> 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</span >
typo? I think you meant, “is speculated to be”. Also it’d be clearer if you replace “object we” with “object, then we”.
<span class="quote">> Source/JavaScriptCore/ChangeLog:9
> + two values (although in the 32-bit case we must also check the other side</span >
typo: must also check *that*
<span class="quote">> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1234
> + MacroAssembler::Jump op2NotCellJump = m_jit.branchIfNotCell(op2.jsValueRegs());</span >
Per our offline discussion, this is incorrect if (taken == nextBlock()). Let’s not do the swap thing. Let’s just have 2 different cases.
<span class="quote">> Source/JavaScriptCore/jsc.cpp:884
> + JSValue cell = exec->argument(0);</span >
nit: I think it’s better to call this “value” instead of “cell” since we don’t know if it’s actually a cell.
<span class="quote">> Source/JavaScriptCore/jsc.cpp:888
> + int64_t asNumber = reinterpret_cast<int64_t>(cell.asCell());</span >
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).
<span class="quote">> Source/JavaScriptCore/tests/stress/equality-type-checking.js:18
> +function dfgify(obj, addr) {
> +
> +
> +}</span >
Please delete.
<span class="quote">> Source/JavaScriptCore/tests/stress/equality-type-checking.js:22
> +</span >
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.</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>