Issue 11564029: Implement Function.apply in vm (issue 5670).

Issue 11564029: Implement Function.apply in vm (issue 5670). (Closed)

Created:
8 years ago by regis

Modified:
8 years ago

Reviewers:
srdjan, siva

CC:
reviews_dartlang.org

Base URL:
http://dart.googlecode.com/svn/branches/bleeding_edge/dart/

Visibility:
Public.

More Reviews

Description

Implement Function.apply in vm (issue 5670). Fix a closure parameter count check bug (unveiled by an apply test). Committed: https://code.google.com/p/dart/source/detail?r=16175

Patch Set 3 : #

Created: 8 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+189 lines, -32 lines)			Patch
M	runtime/lib/array.dart	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
A	runtime/lib/function.cc	View	1 2	1 chunk	+71 lines, -0 lines	0 comments	Download
M	runtime/lib/function_patch.dart	View	1 2	1 chunk	+20 lines, -1 line	0 comments	Download
M	runtime/lib/lib_sources.gypi	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	runtime/vm/bootstrap_natives.h	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	runtime/vm/code_generator.cc	View	1 2	4 chunks	+5 lines, -6 lines	0 comments	Download
M	runtime/vm/dart_api_impl.cc	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	runtime/vm/dart_entry.cc	View	1 2	2 chunks	+30 lines, -9 lines	0 comments	Download
M	runtime/vm/flow_graph_compiler_ia32.cc	View	1 2	3 chunks	+11 lines, -5 lines	0 comments	Download
M	runtime/vm/flow_graph_compiler_x64.cc	View	1 2	3 chunks	+11 lines, -5 lines	0 comments	Download
M	runtime/vm/object.h	View	1 2	1 chunk	+6 lines, -0 lines	0 comments	Download
M	runtime/vm/object.cc	View	1 2	1 chunk	+31 lines, -0 lines	0 comments	Download
M	tests/corelib/corelib.status	View	1 2	2 chunks	+0 lines, -4 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

srdjan

LGTM https://codereview.chromium.org/11564029/diff/6002/runtime/lib/function.cc File runtime/lib/function.cc (right): https://codereview.chromium.org/11564029/diff/6002/runtime/lib/function.cc#newcode18 runtime/lib/function.cc:18: GET_NON_NULL_NATIVE_ARGUMENT(Array, fun_arg_names, arguments->NativeArgAt(1)); This can be CheckedHandles since ...

8 years ago (2012-12-14 18:13:13 UTC) #2

siva

LGTM with some minor nits. https://codereview.chromium.org/11564029/diff/6002/runtime/lib/function_patch.dart File runtime/lib/function_patch.dart (right): https://codereview.chromium.org/11564029/diff/6002/runtime/lib/function_patch.dart#newcode14 runtime/lib/function_patch.dart:14: int numNamedArguments = (?namedArguments ...

8 years ago (2012-12-14 18:30:25 UTC) #3

regis

8 years ago (2012-12-14 18:49:49 UTC) #4

Thanks!

https://codereview.chromium.org/11564029/diff/6002/runtime/lib/function.cc
File runtime/lib/function.cc (right):

https://codereview.chromium.org/11564029/diff/6002/runtime/lib/function.cc#ne...
runtime/lib/function.cc:18: GET_NON_NULL_NATIVE_ARGUMENT(Array, fun_arg_names,
arguments->NativeArgAt(1));
On 2012/12/14 18:13:13, srdjan wrote:
> This can be CheckedHandles since it is an internal/private method which we
> always call with arrays.

Done.

https://codereview.chromium.org/11564029/diff/6002/runtime/lib/function_patch...
File runtime/lib/function_patch.dart (right):

https://codereview.chromium.org/11564029/diff/6002/runtime/lib/function_patch...
runtime/lib/function_patch.dart:14: int numNamedArguments = (?namedArguments &&
(namedArguments != null)) ?
On 2012/12/14 18:30:25, siva wrote:
> would just a null check be sufficient for namedArguments?

Done.

https://codereview.chromium.org/11564029/diff/6002/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/11564029/diff/6002/runtime/vm/code_generator....
runtime/vm/code_generator.cc:769: Exceptions::PropagateError(error);
On 2012/12/14 18:30:25, siva wrote:
> PropagateError already has an UNREACHABLE().

Done.

https://codereview.chromium.org/11564029/diff/6002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/11564029/diff/6002/runtime/vm/object.cc#newco...
runtime/vm/object.cc:8513: if (function != NULL) {
On 2012/12/14 18:30:25, siva wrote:
> You always seem to pass in a function and context handle pointer, do you see a
> case where you might want to call this with null values.
> 
> I am wondering whether we can convert these if (..) checks to ASSERTS.

See vm/dart_api_impl.cc:2453

https://codereview.chromium.org/11564029/diff/6002/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/11564029/diff/6002/runtime/vm/object.h#newcod...
runtime/vm/object.h:3195: // (if not NULL) to call and the context (if not NULL)
to pass.
On 2012/12/14 18:13:13, srdjan wrote:
> to pass to the function.

Done.

Expand Messages | Collapse Messages