<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#c71">Comment # 71</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:ticaiolima@gmail.com" title="Caio Lima <ticaiolima@gmail.com>"> <span class="fn">Caio Lima</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
>> + cannot be deleted when argument "i" is {configurable: false}.
>
> 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 >
OK.
<span class="quote">>> Source/JavaScriptCore/ChangeLog:13
>> + The current implementation is against to the specification for 2 reasons:
>
> 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 >
Ok.
<span class="quote">>> Source/JavaScriptCore/runtime/DirectArguments.cpp:90
>> + size_t configurablesSize = thisObject->m_configurableMap ? thisObject->m_length : 0;
>
> 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 >
Agreed.
<span class="quote">>> Source/JavaScriptCore/runtime/GenericArguments.h:66
>> + static bool canDeletePropertyByIndex(Type*, ExecState*, unsigned);
>
> These aren't implemented anywhere. Please delete.</span >
Nice catch. Sorry.
<span class="quote">>> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:212
>> + thisObject->setConfigurable(vm, index, descriptor.configurable());
>
> 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 >
My guess is that ```Object.defineProperty(arguments, 0, {configurable:true, writable:true, value:50});``` throws TypeError, since we call ```Base::defineOwnProperty``` afterwards. However, I suspect that
```Object.defineProperty(arguments, 0, {configurable:true, writable:true, get: () => {return 50}});``` works and should throw a TypeError instead.
<span class="quote">>> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:214
>> + // Just overrride arguments (i.e finish aliasing) if its descriptor contains {writable: false}.
>
> What do you mean by "finish aliasing" here?</span >
I mean stop mapping. I am changing aliasing from all places that I put it.
<span class="quote">>> JSTests/microbenchmarks/super-getter.js:1
>> +class B {
>
> Please remove this file.</span >
Sorry...</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>