Make ClassFinalizer indifferent on whether we are generating a snapshot or not.

Make ClassFinalizer indifferent on whether we are generating a snapshot or not.
Add flag --error_on_malformed_type.
Remove obsolete flag --verify_implements.
Support snapshotting of LanguageError (for malformed types) and ApiError.
Committed: https://code.google.com/p/dart/source/detail?r=11198

[regis, 2012-08-21 23:53:03]

There is a bit for everyone :-)

Todd, this should help you detect errors earlier when generating snapshots, in
particular malformed types.

Matthias, the parser, compiler, and their APIs were slightly affected.

Ivan, the flag --verify_implements is deleted.

Srdjan and Siva, I do not want you to be jealous. You can have a look too.

Thanks,
Regis

[hausner, 2012-08-22 00:15:39]

finalizer/compiler/parser LGTM w/comments.

https://chromiumcodereview.appspot.com/10871005/diff/1/runtime/vm/class_final...
File runtime/vm/class_finalizer.h (right):

https://chromiumcodereview.appspot.com/10871005/diff/1/runtime/vm/class_final...
runtime/vm/class_finalizer.h:32: kGeneratingSnapshot = true,
Shouldn't the constants be named for what they affect (whether or not to
tolerate malformed types)?

As usual, I find the redefinitions of true and false unhelpful in general.

https://chromiumcodereview.appspot.com/10871005/diff/1/runtime/vm/class_final...
runtime/vm/class_finalizer.h:35: explicit ClassFinalizer(bool
generating_snapshot)
cf above. I would call this parameter "tolerate_malformed_types" or something
similar.

https://chromiumcodereview.appspot.com/10871005/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/10871005/diff/1/runtime/vm/parser.cc#n...
runtime/vm/parser.cc:3595: ErrorMsg(interfaces_pos,
I think you could emit the error message in AddInterfaceIfUnique directly, now
that you hoisted that function into the parser.

[regis, 2012-08-22 02:24:26]

PTAL

Thanks,
Regis

https://chromiumcodereview.appspot.com/10871005/diff/1/runtime/vm/class_final...
File runtime/vm/class_finalizer.h (right):

https://chromiumcodereview.appspot.com/10871005/diff/1/runtime/vm/class_final...
runtime/vm/class_finalizer.h:32: kGeneratingSnapshot = true,
On 2012/08/22 00:15:39, hausner wrote:
> Shouldn't the constants be named for what they affect (whether or not to
> tolerate malformed types)?
> 
> As usual, I find the redefinitions of true and false unhelpful in general.

Agreed. Note that the enum predated this cl. Also, I could not come up with a
better name. I thought of "strict_checking", which would apply nicely to
checking malformed types, but would have the reverse meaning for checking native
resolvers, which we only do when NOT generating a snapshot. I am open to
suggestions :-)

Regarding the style, I can live without enum and with repeating the name of the
formal parameter in a comment at the call site:
class_finalizer(/* strict_checking = */ true);
But there is always a danger for the comments to become obsolete.

https://chromiumcodereview.appspot.com/10871005/diff/1/runtime/vm/class_final...
runtime/vm/class_finalizer.h:35: explicit ClassFinalizer(bool
generating_snapshot)
On 2012/08/22 00:15:39, hausner wrote:
> cf above. I would call this parameter "tolerate_malformed_types" or something
> similar.

Would it be strange to check for native resolver only if !strict_checking?

https://chromiumcodereview.appspot.com/10871005/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/10871005/diff/1/runtime/vm/parser.cc#n...
runtime/vm/parser.cc:3595: ErrorMsg(interfaces_pos,
On 2012/08/22 00:15:39, hausner wrote:
> I think you could emit the error message in AddInterfaceIfUnique directly, now
> that you hoisted that function into the parser.

Done, but I needed to make AddInterfaceIfUnique an instance function.

[regis, 2012-08-22 23:31:43]

Please, have another look.

Note that it was not necessary to make the class finalizer an instance after
all.

The finalizer does not need to know whether we are generating a snapshot or not
to verify that native resolvers are setup for native fields.

Error reporting on malformed types is now explicitly controlled by a vm flag.

Thanks,
Regis

[turnidge, 2012-08-22 23:49:17]

lgtm, for my parts of the review.

Didn't look to closely at the function which moved from the finalizer to the
parser.  Otherwise seems good.

https://chromiumcodereview.appspot.com/10871005/diff/12004/runtime/vm/raw_obj...
File runtime/vm/raw_object_snapshot.cc (right):

https://chromiumcodereview.appspot.com/10871005/diff/12004/runtime/vm/raw_obj...
runtime/vm/raw_object_snapshot.cc:420: ASSERT((kind != Snapshot::kMessage) &&
Why the extra parens?

[siva, 2012-08-22 23:55:04]

LGTM with one comment regarding mismatched read/write on the type parameters.

https://chromiumcodereview.appspot.com/10871005/diff/12004/runtime/vm/raw_obj...
File runtime/vm/raw_object_snapshot.cc (right):

https://chromiumcodereview.appspot.com/10871005/diff/12004/runtime/vm/raw_obj...
runtime/vm/raw_object_snapshot.cc:235: SnapshotWriterVisitor visitor(writer);
We should probably leave it as , false here because the reader is expecting to
read the fields inline. See the read method above.

https://chromiumcodereview.appspot.com/10871005/diff/12004/runtime/vm/raw_obj...
runtime/vm/raw_object_snapshot.cc:290: SnapshotWriterVisitor visitor(writer);
Ditto.

https://chromiumcodereview.appspot.com/10871005/diff/12004/runtime/vm/raw_obj...
runtime/vm/raw_object_snapshot.cc:410: SnapshotWriterVisitor visitor(writer);
Ditto.

[regis, 2012-08-23 00:24:25]

Thanks!

https://chromiumcodereview.appspot.com/10871005/diff/12004/runtime/vm/raw_obj...
File runtime/vm/raw_object_snapshot.cc (right):

https://chromiumcodereview.appspot.com/10871005/diff/12004/runtime/vm/raw_obj...
runtime/vm/raw_object_snapshot.cc:235: SnapshotWriterVisitor visitor(writer);
On 2012/08/22 23:55:05, asiva wrote:
> We should probably leave it as , false here because the reader is expecting to
> read the fields inline. See the read method above.

I fixed it the other way around, by reading refs.

https://chromiumcodereview.appspot.com/10871005/diff/12004/runtime/vm/raw_obj...
runtime/vm/raw_object_snapshot.cc:290: SnapshotWriterVisitor visitor(writer);
On 2012/08/22 23:55:05, asiva wrote:
> Ditto.

Ditto

https://chromiumcodereview.appspot.com/10871005/diff/12004/runtime/vm/raw_obj...
runtime/vm/raw_object_snapshot.cc:410: SnapshotWriterVisitor visitor(writer);
On 2012/08/22 23:55:05, asiva wrote:
> Ditto.

Ditto

https://chromiumcodereview.appspot.com/10871005/diff/12004/runtime/vm/raw_obj...
runtime/vm/raw_object_snapshot.cc:420: ASSERT((kind != Snapshot::kMessage) &&
On 2012/08/22 23:49:17, turnidge wrote:
> Why the extra parens?

Not that I like them that much, but our style guide requires them. I try to add
them when I touch the code.