[Webkit-unassigned] [Bug 159398] [test262] Fixing mapped arguments object property test case

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=159398

--- Comment #70 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 290583
  --> https://bugs.webkit.org/attachment.cgi?id=290583
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        property. Also it is fixing cases where arguments[i]
> +        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"

> 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.

> 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

I'm confused by what you mean here.

> Source/JavaScriptCore/ChangeLog:22
> +        2. When a property is overriden it is always returning true. However

I would say "delete" instead of "it"

> 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.

> Source/JavaScriptCore/runtime/GenericArguments.h:66
> +    static bool canDeletePropertyById(Type*, ExecState*, PropertyName);
> +    static bool canDeletePropertyByIndex(Type*, ExecState*, unsigned);

These aren't implemented anywhere. Please delete.

> 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")

> 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?

> JSTests/microbenchmarks/super-getter.js:1
> +class B {

Please remove this file.

-- 
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/20161009/c9c138e5/attachment.html>