<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [test262] Fixing mapped arguments object property test case"
href="https://bugs.webkit.org/show_bug.cgi?id=159398#c70">Comment # 70</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [test262] Fixing mapped arguments object property test case"
href="https://bugs.webkit.org/show_bug.cgi?id=159398">bug 159398</a>
from <span class="vcard"><a class="email" href="mailto:sbarati@apple.com" title="Saam Barati <sbarati@apple.com>"> <span class="fn">Saam Barati</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=290583&action=diff" name="attach_290583" title="Patch">attachment 290583</a> <a href="attachment.cgi?id=290583&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=290583&action=review">https://bugs.webkit.org/attachment.cgi?id=290583&action=review</a>
<span class="quote">> Source/JavaScriptCore/ChangeLog:11
> + property. Also it is fixing cases where arguments[i]
> + cannot be deleted when argument "i" is {configurable: false}.</span >
I think you mean you're making it so that arguments[i] can't be deleted, correct? I would maybe write "fixing cases where" => "ensuring that"
<span class="quote">> Source/JavaScriptCore/ChangeLog:13
> + The current implementation is against to the specification for 2 reasons:</span >
I would say "previous" instead of "current" if you're referring to the behavior that is ToT now and what will be the old behavior as your patch lands.
<span class="quote">> Source/JavaScriptCore/ChangeLog:18
> + It means that we just stop aliasing an property index already
> + defined if its property descriptor contains writable and its value</span >
I'm confused by what you mean here.
<span class="quote">> Source/JavaScriptCore/ChangeLog:22
> + 2. When a property is overriden it is always returning true. However</span >
I would say "delete" instead of "it"
<span class="quote">> Source/JavaScriptCore/runtime/DirectArguments.cpp:90
> + size_t configurablesSize = thisObject->m_configurableMap ? thisObject->m_length : 0;</span >
Please make this * sizeof(bool). I would also argue that the overridesSize variable above should do the same. I'm not a fan of the style assuming what sizeof(bool) is.
<span class="quote">> Source/JavaScriptCore/runtime/GenericArguments.h:66
> + static bool canDeletePropertyById(Type*, ExecState*, PropertyName);
> + static bool canDeletePropertyByIndex(Type*, ExecState*, unsigned);</span >
These aren't implemented anywhere. Please delete.
<span class="quote">> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:212
> + thisObject->setConfigurable(vm, index, descriptor.configurable());</span >
We're guaranteed that it's already configurable before we call this? It seems like maybe we should bail out somewhere if the property is already non-configurable.
If i had to guess, maybe this program exhibits the bug:
function foo(x) {
Object.defineProperty(arguments, 0, {configurable:false, writable:true, value:20});
Object.defineProperty(arguments, 0, {configurable:true, writable:true, value:50}); // Does this succeed?
}
foo("foo")
<span class="quote">> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:214
> + // Just overrride arguments (i.e finish aliasing) if its descriptor contains {writable: false}.</span >
What do you mean by "finish aliasing" here?
<span class="quote">> JSTests/microbenchmarks/super-getter.js:1
> +class B {</span >
Please remove this file.</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>