[webkit-changes] cvs commit: JavaScriptCore/kjs function.cpp function.h

[Maciej Stachowiak]

On Jun 14, 2005, at 9:36 AM, Geoffrey wrote:

> ggaren      05/06/14 09:36:11
>
>   Modified:    .        ChangeLog
>                kjs      function.cpp function.h
>   Log:
>       Fixed: <rdar://problem/4147745> JavaScript discards locally  
> defined "arguments" property
>
>       No layout tests added because this change fixes existing tests:
>       ecma/ExecutionContexts/10.1.6.js
>           ecma_3/Function/regress-94506.js
>           js1_4/Functions/function-001.js
>
>           Reviewed by cblu.
>
>           * kjs/function.cpp:
>           (KJS::ActivationImp::get): get now checks for an  
> "arguments" property defined in the local variable object
>       before trying to return the built-in arguments array.
>
>           * kjs/function.h: ActivationImp::put no longer overrides  
> ObjectImp::put

1) OK, I owe you a beer. How about at lunch tomorrow?

2) A comment on the change below:


>   Revision  Changes    Path
>   1.710     +17 -0     JavaScriptCore/ChangeLog
>
>   Index: ChangeLog
>   ===================================================================
>   RCS file: /cvs/root/JavaScriptCore/ChangeLog,v
>   retrieving revision 1.709
>   retrieving revision 1.710
>   diff -u -r1.709 -r1.710
>   --- ChangeLog    10 Jun 2005 18:02:32 -0000    1.709
>   +++ ChangeLog    14 Jun 2005 16:36:10 -0000    1.710
>   @@ -1,3 +1,20 @@
>   +2005-06-14  Geoffrey Garen  <ggaren at apple.com>
>   +
>   +    Fixed: <rdar://problem/4147745> JavaScript discards locally  
> defined "arguments" property
>   +
>   +    No layout tests added because this change fixes existing tests:
>   +    ecma/ExecutionContexts/10.1.6.js
>   +        ecma_3/Function/regress-94506.js
>   +        js1_4/Functions/function-001.js
>   +
>   +        Reviewed by cblu.
>   +
>   +        * kjs/function.cpp:
>   +        (KJS::ActivationImp::get): get now checks for an  
> "arguments" property defined in the local variable object
>   +    before trying to return the built-in arguments array.
>   +
>   +        * kjs/function.h: ActivationImp::put no longer overrides  
> ObjectImp::put
>   +
>    2005-06-10  Darin Adler  <darin at apple.com>
>
>            Change by Mark Rowe <opendarwin.org at bdash.net.nz>.
>
>
>
>   1.39      +7 -10     JavaScriptCore/kjs/function.cpp
>
>   Index: function.cpp
>   ===================================================================
>   RCS file: /cvs/root/JavaScriptCore/kjs/function.cpp,v
>   retrieving revision 1.38
>   retrieving revision 1.39
>   diff -u -r1.38 -r1.39
>   --- function.cpp    16 Mar 2005 18:30:25 -0000    1.38
>   +++ function.cpp    14 Jun 2005 16:36:11 -0000    1.39
>   @@ -344,22 +344,19 @@
>    Value ActivationImp::get(ExecState *exec, const Identifier  
> &propertyName) const
>    {
>        if (propertyName == argumentsPropertyName) {
>   +        // check for locally declared arguments property
>   +        ValueImp *v = getDirect(propertyName);
>   +        if (v)
>   +            return Value(v);
>   +
>   +        // default: return builtin arguments array
>            if (!_argumentsObject)
>   -            createArgumentsObject(exec);
>   +                createArgumentsObject(exec);
>            return Value(_argumentsObject);
>        }
>        return ObjectImp::get(exec, propertyName);
>    }

Generally I would say it's better to do the getDirect(propertyName)  
outside if (propertyname == argumentsPropertyName), as that will be  
more appropriate in cases where you need to allow local override but  
there is more than one special property like this. It's not really  
specific to the arguments property, but rather just a general  
principle that you want to check overrides before special properties.

Now, if you did that, it would lead to getDirect() being called twice  
in some cases, since ObjectImp::get already calls it. This could  
cause a performance problem. You could check this by running the cvs- 
js-ibench PLT suite with and without the change. If it does slow  
things down, it might be useful to make a variant of ObjectImp::get  
that assumes getDirect has already been called and so does not call  
it again. In fact, the Window object could also use this.

>
>   -void ActivationImp::put(ExecState *exec, const Identifier  
> &propertyName, const Value &value, int attr)
>   -{
>   -    if (propertyName == argumentsPropertyName) {
>   -        // FIXME: Do we need to allow overwriting this?
>   -        return;
>   -    }
>   -    ObjectImp::put(exec, propertyName, value, attr);
>   -}
>   -
>    bool ActivationImp::hasProperty(ExecState *exec, const  
> Identifier &propertyName) const
>    {
>        if (propertyName == argumentsPropertyName)
>
>
>
>   1.20      +0 -1      JavaScriptCore/kjs/function.h
>
>   Index: function.h
>   ===================================================================
>   RCS file: /cvs/root/JavaScriptCore/kjs/function.h,v
>   retrieving revision 1.19
>   retrieving revision 1.20
>   diff -u -r1.19 -r1.20
>   --- function.h    18 Aug 2003 18:51:25 -0000    1.19
>   +++ function.h    14 Jun 2005 16:36:11 -0000    1.20
>   @@ -104,7 +104,6 @@
>        ActivationImp(FunctionImp *function, const List &arguments);
>
>        virtual Value get(ExecState *exec, const Identifier  
> &propertyName) const;
>   -    virtual void put(ExecState *exec, const Identifier  
> &propertyName, const Value &value, int attr = None);
>        virtual bool hasProperty(ExecState *exec, const Identifier  
> &propertyName) const;
>        virtual bool deleteProperty(ExecState *exec, const  
> Identifier &propertyName);
>
>
>
>
> _______________________________________________
> webkit-changes mailing list
> webkit-changes at opendarwin.org
> http://www.opendarwin.org/mailman/listinfo/webkit-changes
>