Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(190)

Issue 10916039: Throw AbstractClassInstantiationError if an abstract class is instantiated (Closed)

Created:
8 years, 3 months ago by hausner
Modified:
8 years, 3 months ago
Reviewers:
regis
CC:
reviews_dartlang.org, kasperl
Visibility:
Public.

Description

Throw AbstractClassInstantiationError if an abstract class is instantiated This is the second try to my previous attempt to implement the new abstract class handling. Instead of a compile-time error, the VM now throws an AbstractClassInstantiationError at runtime if an abstract class is instantiated. Added the new error class to the core library. For now I'm still using the rule that a class is abstract if it is explicitly marked as abstract, or if it defines a new abstract method. Committed: https://code.google.com/p/dart/source/detail?r=11708

Patch Set 1 #

Total comments: 12

Patch Set 2 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+207 lines, -61 lines) Patch
M lib/core/errors.dart View 1 1 chunk +3 lines, -0 lines 0 comments Download
M runtime/lib/error.cc View 1 1 chunk +32 lines, -0 lines 0 comments Download
M runtime/lib/error.dart View 1 1 chunk +22 lines, -0 lines 0 comments Download
M runtime/vm/bootstrap_natives.h View 1 2 chunks +2 lines, -1 line 0 comments Download
M runtime/vm/object.h View 1 2 chunks +8 lines, -1 line 0 comments Download
M runtime/vm/object.cc View 1 1 chunk +5 lines, -0 lines 0 comments Download
M runtime/vm/parser.cc View 1 10 chunks +70 lines, -22 lines 0 comments Download
M runtime/vm/symbols.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M tests/co19/co19-runtime.status View 1 2 chunks +40 lines, -6 lines 0 comments Download
D tests/language/abstract_factory_constructor_test.dart View 1 1 chunk +5 lines, -5 lines 0 comments Download
M tests/language/abstract_getter_test.dart View 1 1 chunk +1 line, -1 line 0 comments Download
D tests/language/empty_body_member_negative_test.dart View 1 1 chunk +0 lines, -15 lines 0 comments Download
M tests/language/get_set_syntax_test.dart View 1 3 chunks +7 lines, -5 lines 0 comments Download
M tests/language/implicit_this_test.dart View 1 1 chunk +2 lines, -2 lines 0 comments Download
A + tests/language/interface_negative_test.dart View 0 chunks +-1 lines, --1 lines 0 comments Download
D tests/language/interface_test.dart View 1 1 chunk +1 line, -2 lines 0 comments Download
M tests/language/language.status View 1 2 chunks +1 line, -1 line 0 comments Download
M tests/language/language_dart2js.status View 1 2 chunks +7 lines, -1 line 0 comments Download

Messages

Total messages: 3 (0 generated)
hausner
I wouldn't want you to be idle while you wait for me to review your ...
8 years, 3 months ago (2012-08-30 23:40:14 UTC) #1
regis
LGTM with comments and still a bit confused why most of your tests are marked ...
8 years, 3 months ago (2012-08-31 01:16:48 UTC) #2
hausner
8 years, 3 months ago (2012-08-31 18:06:52 UTC) #3
Thanks for the review!

http://codereview.chromium.org/10916039/diff/1/runtime/lib/error.cc
File runtime/lib/error.cc (right):

http://codereview.chromium.org/10916039/diff/1/runtime/lib/error.cc#newcode106
runtime/lib/error.cc:106: 
On 2012/08/31 01:16:48, regis wrote:
> Missing blank line.
> Also, missing comment documenting arguments, as all other runtime entries do. 

Done.

http://codereview.chromium.org/10916039/diff/1/runtime/lib/error.cc#newcode133
runtime/lib/error.cc:133: 
On 2012/08/31 01:16:48, regis wrote:
> Missing blank line.

Done.

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

http://codereview.chromium.org/10916039/diff/1/runtime/vm/bootstrap_natives.h...
runtime/vm/bootstrap_natives.h:204: #define DECLARE_BOOTSTRAP_NATIVE(name,
ignored)         \
On 2012/08/31 01:16:48, regis wrote:
> While you are here, you could move this backslash to column 80.

Done.

http://codereview.chromium.org/10916039/diff/1/tests/language/abstract_factor...
File tests/language/abstract_factory_constructor_test.dart (left):

http://codereview.chromium.org/10916039/diff/1/tests/language/abstract_factor...
tests/language/abstract_factory_constructor_test.dart:28: }
On 2012/08/31 01:16:48, regis wrote:
> Why not turn this test into a negative one instead of deleting it?
> /// 01: runtime error

We have other tests that make sure we cannot instantiate an abstract class.  But
I agree that the static type warning may still be a valuable test case.

http://codereview.chromium.org/10916039/diff/1/tests/language/get_set_syntax_...
File tests/language/get_set_syntax_test.dart (right):

http://codereview.chromium.org/10916039/diff/1/tests/language/get_set_syntax_...
tests/language/get_set_syntax_test.dart:48: new C1();           /// 16:
compile-time error
On 2012/08/31 01:16:48, regis wrote:
> I thought it was now a runtime error?

Correct. My oversight.

http://codereview.chromium.org/10916039/diff/1/tests/language/interface_test....
File tests/language/interface_test.dart (left):

http://codereview.chromium.org/10916039/diff/1/tests/language/interface_test....
tests/language/interface_test.dart:40: var o = new Bi(); /// static type warning
On 2012/08/31 01:16:48, regis wrote:
> /// runtime error?

Revived the test. Done.

Powered by Google App Engine
This is Rietveld 408576698