Finalize all classes (fix issue 364).

Finalize all classes (fix issue 364).
Committed: https://code.google.com/p/dart/source/detail?r=1380

[srdjan, 2011-11-09 21:22:43]

LGTM with suggestions

http://codereview.chromium.org/8506001/diff/1/runtime/vm/class_finalizer.cc
File runtime/vm/class_finalizer.cc (right):

http://codereview.chromium.org/8506001/diff/1/runtime/vm/class_finalizer.cc#n...
runtime/vm/class_finalizer.cc:21: void ClassFinalizer::ExpectPendingClasses() {
Do you want to allow ExpectPendingClasses several times before finalizing? I.e.,
do you need to assert !AllClassFinalized().

http://codereview.chromium.org/8506001/diff/1/runtime/vm/class_finalizer.cc#n...
runtime/vm/class_finalizer.cc:35: }
Why the code above?

http://codereview.chromium.org/8506001/diff/1/runtime/vm/class_finalizer.cc#n...
runtime/vm/class_finalizer.cc:274: "cannot resolve name '%s'\n",
Maybe be more specific -> cannot resolve library prefix.

http://codereview.chromium.org/8506001/diff/1/runtime/vm/class_finalizer.cc#n...
runtime/vm/class_finalizer.cc:607: if (type.IsSignatureType() &&
!AllClassesFinalized()) {
Maybe have a interim method: CollectingClassesToFinalize ->
!AllClassesFinalized?

http://codereview.chromium.org/8506001/diff/1/runtime/vm/class_finalizer.h
File runtime/vm/class_finalizer.h (right):

http://codereview.chromium.org/8506001/diff/1/runtime/vm/class_finalizer.h#ne...
runtime/vm/class_finalizer.h:41: // Notify finalizer to expect classes to be
finalized.
Maybe add somewhere the comment that we are flagging the interval between
ExpectPendingClasses and FinalizePendingClasses.

http://codereview.chromium.org/8506001/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/8506001/diff/1/runtime/vm/object.cc#newcode344
runtime/vm/object.cc:344: GrowableArray<const Class*>* classes) {
I think it would be clearer if classes->Add() would be done at call site and
'classes' would not be a n argument.

http://codereview.chromium.org/8506001/diff/1/runtime/vm/object.cc#newcode358
runtime/vm/object.cc:358: GrowableArray<const Class*>* classes) {
Ditto

[regis, 2011-11-09 23:31:01]

Thanks!

http://codereview.chromium.org/8506001/diff/1/runtime/vm/class_finalizer.cc
File runtime/vm/class_finalizer.cc (right):

http://codereview.chromium.org/8506001/diff/1/runtime/vm/class_finalizer.cc#n...
runtime/vm/class_finalizer.cc:21: void ClassFinalizer::ExpectPendingClasses() {
On 2011/11/09 21:22:43, srdjan wrote:
> Do you want to allow ExpectPendingClasses several times before finalizing?
I.e.,
> do you need to assert !AllClassFinalized().

Good question!
Currently, there may be several calls to ExpectPendingClasses (e.g.
Object::Init() and Parser::ParseTopLevel()) before a call to
FinalizePendingClasses. We could force proper pairing of these calls, but we
would probably need to use a counter rather than a binary flag to allow proper
nesting.
I prefer to allow multiple calls for now.

http://codereview.chromium.org/8506001/diff/1/runtime/vm/class_finalizer.cc#n...
runtime/vm/class_finalizer.cc:35: }
On 2011/11/09 21:22:43, srdjan wrote:
> Why the code above?

Left over debugging code. Removed.

http://codereview.chromium.org/8506001/diff/1/runtime/vm/class_finalizer.cc#n...
runtime/vm/class_finalizer.cc:274: "cannot resolve name '%s'\n",
On 2011/11/09 21:22:43, srdjan wrote:
> Maybe be more specific -> cannot resolve library prefix.

Done.

http://codereview.chromium.org/8506001/diff/1/runtime/vm/class_finalizer.cc#n...
runtime/vm/class_finalizer.cc:607: if (type.IsSignatureType() &&
!AllClassesFinalized()) {
On 2011/11/09 21:22:43, srdjan wrote:
> Maybe have a interim method: CollectingClassesToFinalize ->
> !AllClassesFinalized?

Done. Renamed these calls to:
ExpectClassesToFinalize
IsExpectingClassesToFinalize
AddClassesToFinalize
AllClassesFinalized
FinalizeAllClasses

http://codereview.chromium.org/8506001/diff/1/runtime/vm/class_finalizer.h
File runtime/vm/class_finalizer.h (right):

http://codereview.chromium.org/8506001/diff/1/runtime/vm/class_finalizer.h#ne...
runtime/vm/class_finalizer.h:41: // Notify finalizer to expect classes to be
finalized.
On 2011/11/09 21:22:43, srdjan wrote:
> Maybe add somewhere the comment that we are flagging the interval between
> ExpectPendingClasses and FinalizePendingClasses.

Done.

http://codereview.chromium.org/8506001/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/8506001/diff/1/runtime/vm/object.cc#newcode344
runtime/vm/object.cc:344: GrowableArray<const Class*>* classes) {
On 2011/11/09 21:22:43, srdjan wrote:
> I think it would be clearer if classes->Add() would be done at call site and
> 'classes' would not be a n argument. 

Done.

http://codereview.chromium.org/8506001/diff/1/runtime/vm/object.cc#newcode358
runtime/vm/object.cc:358: GrowableArray<const Class*>* classes) {
On 2011/11/09 21:22:43, srdjan wrote:
> Ditto

Done.