Implement Function.prototype.bind (ES5 15.3.4.5)....

Implement Function.prototype.bind (ES5 15.3.4.5).

Please note that we do not implement correctly the setting of caller
and arguments on the returned objects, since we already have these
properties on function objects (and they are non-configurable).

Also corrects indention in DefineOwnProperty.


Committed: http://code.google.com/p/v8/source/detail?r=5124

[Mads Ager (chromium), 2010-07-23 06:31:27]

LGTM with these comments addressed.

http://codereview.chromium.org/3046010/diff/10001/11003
File src/v8natives.js (right):

http://codereview.chromium.org/3046010/diff/10001/11003#newcode1126
src/v8natives.js:1126: args.unshift.apply(args, boundArgs);
Could we allocate a bigger array up front and fill it with boundArgs and args
instead of using unshift here?

http://codereview.chromium.org/3046010/diff/10001/11003#newcode1134
src/v8natives.js:1134: } else {
We should be able to get rid of the else here. There is little difference
between if and else branch.

http://codereview.chromium.org/3046010/diff/10001/11003#newcode1150
src/v8natives.js:1150: // try to redefine these as defined by the spec.
Could you add to the comment what the spec says?

http://codereview.chromium.org/3046010/diff/10001/11003#newcode1153
src/v8natives.js:1153: %FunctionSetLength(result, $Math.max(0, this.length -
(n-1)));
Math.max can be overwritten. Use simple conditional instead.

http://codereview.chromium.org/3046010/diff/10001/11004
File test/mjsunit/function-bind.js (right):

http://codereview.chromium.org/3046010/diff/10001/11004#newcode158
test/mjsunit/function-bind.js:158: assertFalse(obj2 instanceof f);
Add tests for the lengths of bound functions?

[Rico, 2010-07-23 07:45:14]

Mads, please take another look.

Changed thisArg to this_arg and boundArgs to bound_args.
Refactored argument counts to simplify both binding and length calculations.
Added tests for lengths.

http://codereview.chromium.org/3046010/diff/10001/11003
File src/v8natives.js (right):

http://codereview.chromium.org/3046010/diff/10001/11003#newcode1126
src/v8natives.js:1126: args.unshift.apply(args, boundArgs);
On 2010/07/23 06:31:29, Mads Ager wrote:
> Could we allocate a bigger array up front and fill it with boundArgs and args
> instead of using unshift here?

Done.

http://codereview.chromium.org/3046010/diff/10001/11003#newcode1134
src/v8natives.js:1134: } else {
On 2010/07/23 06:31:29, Mads Ager wrote:
> We should be able to get rid of the else here. There is little difference
> between if and else branch.

Done.

http://codereview.chromium.org/3046010/diff/10001/11003#newcode1150
src/v8natives.js:1150: // try to redefine these as defined by the spec.
On 2010/07/23 06:31:29, Mads Ager wrote:
> Could you add to the comment what the spec says?

Done.

http://codereview.chromium.org/3046010/diff/10001/11003#newcode1153
src/v8natives.js:1153: %FunctionSetLength(result, $Math.max(0, this.length -
(n-1)));
On 2010/07/23 06:31:29, Mads Ager wrote:
> Math.max can be overwritten. Use simple conditional instead.

Done.

[Mads Ager (chromium), 2010-07-23 08:36:22]

Fix length setting and this LGTM.

http://codereview.chromium.org/3046010/diff/17001/18003
File src/v8natives.js (right):

http://codereview.chromium.org/3046010/diff/17001/18003#newcode1146
src/v8natives.js:1146: %FunctionSetLength(result, this.length - argc_bound);
You do need a check for less than zero here, right? argc_bound can easily be
larger than this.length.

http://codereview.chromium.org/3046010/diff/17001/18004
File test/mjsunit/function-bind.js (right):

http://codereview.chromium.org/3046010/diff/17001/18004#newcode47
test/mjsunit/function-bind.js:47: f= foo.bind(foo, 2, 2, 2);
Add another case where you bind more arguments than foo.length.

Also, space between 'f' and '='.

[Rico, 2010-07-23 10:08:51]

http://codereview.chromium.org/3046010/diff/17001/18003
File src/v8natives.js (right):

http://codereview.chromium.org/3046010/diff/17001/18003#newcode1146
src/v8natives.js:1146: %FunctionSetLength(result, this.length - argc_bound);
On 2010/07/23 08:36:23, Mads Ager wrote:
> You do need a check for less than zero here, right? argc_bound can easily be
> larger than this.length.

Done.

http://codereview.chromium.org/3046010/diff/17001/18004
File test/mjsunit/function-bind.js (right):

http://codereview.chromium.org/3046010/diff/17001/18004#newcode47
test/mjsunit/function-bind.js:47: f= foo.bind(foo, 2, 2, 2);
On 2010/07/23 08:36:23, Mads Ager wrote:
> Add another case where you bind more arguments than foo.length.
> 
> Also, space between 'f' and '='.

Done.

[Lasse Reichstein, 2010-07-25 11:45:10]

Late comment.

http://codereview.chromium.org/3046010/diff/6006/24004
File test/mjsunit/function-bind.js (right):

http://codereview.chromium.org/3046010/diff/6006/24004#newcode183
test/mjsunit/function-bind.js:183: assertFalse(obj2 instanceof f);
This should be assertTrue too (see ES5, 15.3.4.5.3).

[Aaron.Heckmann_gmail.com, 2010-07-27 02:21:47]

global.print throws an error in node.js and should be removed.

http://codereview.chromium.org/3046010/diff/6006/24003
File src/v8natives.js (right):

http://codereview.chromium.org/3046010/diff/6006/24003#newcode1115
src/v8natives.js:1115: global.print(argc_bound);
Looks like this line should be removed?