Issue 9980004: Look up the correct constructor for default classes.

Issue 9980004: Look up the correct constructor for default classes. (Closed)

Created:
8 years, 8 months ago by floitsch

Modified:
8 years, 8 months ago

Reviewers:
ahe

CC:
reviews_dartlang.org

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

Visibility:
Public.

More Reviews

Description

Look up the correct constructor for default classes. Committed: https://code.google.com/p/dart/source/detail?r=6402

Patch Set 2 : Address comments. #

Patch Set 3 : rebase #

Created: 8 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+38 lines, -29 lines)			Patch
M	lib/compiler/implementation/elements/elements.dart	View	1	1 chunk	+10 lines, -0 lines	0 comments	Download
M	lib/compiler/implementation/lib/io.dart	View		3 chunks	+7 lines, -7 lines	0 comments	Download
M	lib/compiler/implementation/lib/mock.dart	View		1 chunk	+1 line, -1 line	0 comments	Download
M	lib/compiler/implementation/lib/mockimpl.dart	View		1 chunk	+1 line, -1 line	0 comments	Download
M	lib/compiler/implementation/resolver.dart	View	1	1 chunk	+18 lines, -16 lines	0 comments	Download
M	lib/compiler/implementation/warnings.dart	View		1 chunk	+1 line, -1 line	0 comments	Download
M	tests/co19/co19-leg.status	View	1 2	2 chunks	+0 lines, -2 lines	0 comments	Download
M	tests/language/language-leg.status	View	1 2	1 chunk	+0 lines, -1 line	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

ahe

LGTM! Only nits below. Cheers, Peter https://chromiumcodereview.appspot.com/9980004/diff/1/lib/compiler/implementation/elements/elements.dart File lib/compiler/implementation/elements/elements.dart (right): https://chromiumcodereview.appspot.com/9980004/diff/1/lib/compiler/implementation/elements/elements.dart#newcode829 lib/compiler/implementation/elements/elements.dart:829: assert(intrface.isInterface()); I don't ...

8 years, 8 months ago (2012-04-10 14:30:38 UTC) #2

floitsch

8 years, 8 months ago (2012-04-11 11:40:56 UTC) #3

https://chromiumcodereview.appspot.com/9980004/diff/1/lib/compiler/implementa...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/9980004/diff/1/lib/compiler/implementa...
lib/compiler/implementation/elements/elements.dart:829:
assert(intrface.isInterface());
On 2012/04/10 14:30:38, ahe wrote:
> I don't think this assertion is correct. We have implicit interfaces from a
> class declaration.

Done.

https://chromiumcodereview.appspot.com/9980004/diff/1/lib/compiler/implementa...
lib/compiler/implementation/elements/elements.dart:830: if (allSupertypes ===
null) {
On 2012/04/10 14:30:38, ahe wrote:
> I don't understand why this is better than a null pointer exception in for-in
> below. The user doesn't care: the compiler crashed and did not give her any
> usable information for trying to locate the problem.

removed.

https://chromiumcodereview.appspot.com/9980004/diff/1/lib/compiler/implementa...
lib/compiler/implementation/elements/elements.dart:831: throw "internal error:
implementsInterface with allSupertypes == null.";
On 2012/04/10 14:30:38, ahe wrote:
> Long line: trailing whitespace?

removed.

https://chromiumcodereview.appspot.com/9980004/diff/1/lib/compiler/implementa...
lib/compiler/implementation/elements/elements.dart:835: if (implementedInterface
== intrface) {
On 2012/04/10 14:30:38, ahe wrote:
> Use ===, not ==.

Done.

https://chromiumcodereview.appspot.com/9980004/diff/1/lib/compiler/implementa...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/9980004/diff/1/lib/compiler/implementa...
lib/compiler/implementation/resolver.dart:175: name = new
SourceString(constructor.name.slowToString().replaceFirst(
On 2012/04/10 14:30:38, ahe wrote:
> Please add "TODO(ahe): Don't use string replacement here".

Done.

Expand Messages | Collapse Messages