Issue 11087070: - Get rid of RawClosure class and use RawInstance for closures.

Issue 11087070: - Get rid of RawClosure class and use RawInstance for closures. (Closed)

Created:
8 years, 2 months ago by siva

Modified:
8 years, 2 months ago

Reviewers:
regis, Ivan Posva

CC:
reviews_dartlang.org, vm-dev_dartlang.org, Tom Ball

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

Visibility:
Public.

More Reviews

Description

- Get rid of RawClosure class and use RawInstance for closures. - Simplify SetRaw to not require an access to the Current Isolate. Committed: https://code.google.com/p/dart/source/detail?r=13581

Patch Set 3 : #

Created: 8 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+146 lines, -158 lines)			Patch
M	lib/isolate.cc	View	1 2	2 chunks	+14 lines, -5 lines	0 comments	Download
M	vm/ast.cc	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download
M	vm/code_generator.cc	View	1 2	6 chunks	+9 lines, -9 lines	0 comments	Download
M	vm/dart_api_impl.cc	View	1 2	2 chunks	+10 lines, -9 lines	0 comments	Download
M	vm/dart_entry.h	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	vm/dart_entry.cc	View	1 2	1 chunk	+3 lines, -3 lines	0 comments	Download
M	vm/object.h	View	1 2	6 chunks	+60 lines, -22 lines	0 comments	Download
M	vm/object.cc	View	1 2	7 chunks	+35 lines, -39 lines	0 comments	Download
M	vm/object_test.cc	View	1 2	2 chunks	+2 lines, -2 lines	0 comments	Download
M	vm/raw_object.h	View	1 2	2 chunks	+0 lines, -14 lines	0 comments	Download
M	vm/raw_object.cc	View	1 2	2 chunks	+2 lines, -17 lines	0 comments	Download
M	vm/raw_object_snapshot.cc	View	1 2	3 chunks	+2 lines, -24 lines	0 comments	Download
M	vm/snapshot.h	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	vm/snapshot.cc	View	1 2	2 chunks	+3 lines, -8 lines	0 comments	Download
M	vm/stub_code_ia32.cc	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	vm/stub_code_x64.cc	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

Ivan Posva

https://chromiumcodereview.appspot.com/11087070/diff/11001/vm/ast.cc File vm/ast.cc (right): https://chromiumcodereview.appspot.com/11087070/diff/11001/vm/ast.cc#newcode290 vm/ast.cc:290: return &Instance::Handle(); This does not match with the comment. ...

8 years, 2 months ago (2012-10-11 04:04:22 UTC) #2

regis

LGTM Nice! I agree with all comments by Ivan. https://chromiumcodereview.appspot.com/11087070/diff/11001/vm/ast.cc File vm/ast.cc (right): https://chromiumcodereview.appspot.com/11087070/diff/11001/vm/ast.cc#newcode290 vm/ast.cc:290: ...

8 years, 2 months ago (2012-10-11 15:21:55 UTC) #3

siva

8 years, 2 months ago (2012-10-11 17:45:58 UTC) #4

https://chromiumcodereview.appspot.com/11087070/diff/11001/vm/ast.cc
File vm/ast.cc (right):

https://chromiumcodereview.appspot.com/11087070/diff/11001/vm/ast.cc#newcode290
vm/ast.cc:290: return &Instance::Handle();
Changed comment to state that we return a dart instance.

On 2012/10/11 04:04:22, Ivan Posva wrote:
> This does not match with the comment. The type of the handle is now an
Instance
> and not a Closure. Please either make sure that the type does not matter and
> update the comment or we will need to have a closure singleton object so that
we
> can check for the object being a closure.

https://chromiumcodereview.appspot.com/11087070/diff/11001/vm/ast.cc#newcode290
vm/ast.cc:290: return &Instance::Handle();
Checked with Mathias, he thinks it is ok to be a Handle as it is discarded
immediately on return.

On 2012/10/11 15:21:55, regis wrote:
> Shouldn't this be a ZoneHandle?

https://chromiumcodereview.appspot.com/11087070/diff/11001/vm/code_generator.cc
File vm/code_generator.cc (right):

https://chromiumcodereview.appspot.com/11087070/diff/11001/vm/code_generator....
vm/code_generator.cc:1158: const Instance& closure =
Instance::CheckedHandle(arguments.At(0));
I will add asserts in 'function', 'context' functions to
check that the passed in instance is a closure.

On 2012/10/11 04:04:22, Ivan Posva wrote:
> We used to check that we are really getting a closure object here. This is not
> the case any longer. Is the code in Closure::function checking that the
argument
> being passed in is a closure?

https://chromiumcodereview.appspot.com/11087070/diff/11001/vm/object.cc
File vm/object.cc (right):

https://chromiumcodereview.appspot.com/11087070/diff/11001/vm/object.cc#newco...
vm/object.cc:8158: const Function& fun =
Function::Handle(Closure::function(*this));
On 2012/10/11 04:04:22, Ivan Posva wrote:
> Could this be abstracted out into a function in the Closure class?

Done.

https://chromiumcodereview.appspot.com/11087070/diff/11001/vm/object.h
File vm/object.h (right):

https://chromiumcodereview.appspot.com/11087070/diff/11001/vm/object.h#newcod...
vm/object.h:5463: return *FunctionAddr(closure);
On 2012/10/11 04:04:22, Ivan Posva wrote:
> ASSERT(closure.IsClosure());

Done.

https://chromiumcodereview.appspot.com/11087070/diff/11001/vm/object.h#newcod...
vm/object.h:5470: return *ContextAddr(closure);
On 2012/10/11 04:04:22, Ivan Posva wrote:
> ditto: ASSERT

Done.

https://chromiumcodereview.appspot.com/11087070/diff/11001/vm/object.h#newcod...
vm/object.h:5504: return reinterpret_cast<RawAbstractTypeArguments**>(
On 2012/10/11 04:04:22, Ivan Posva wrote:
> Actually the ASSERTs might make more sense here.

Done.

Expand Messages | Collapse Messages