Fix for issue #6259: also look for subclasses when checking if a typed selector applies to an eleme…

Fix for issue #6259: also look for subclasses when checking if a typed selector applies to an element.
Committed: https://code.google.com/p/dart/source/detail?r=14208

[ahe, 2012-10-29 06:18:29]

I'm worried about the compile-time overhead. How about we disable this
optimization until we can find a fast way of computing this information?

https://codereview.chromium.org/11315005/diff/1/lib/compiler/implementation/u...
File lib/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/11315005/diff/1/lib/compiler/implementation/u...
lib/compiler/implementation/universe/universe.dart:529: bool
hasOneSubclassThatImplements(ClassElement superClass) {
Have you measured the compile-time impact of this?

[ngeoffray, 2012-10-29 13:04:42]

Thanks for the comment Peter, PTAL

[ahe, 2012-10-29 13:15:21]

https://codereview.chromium.org/11315005/diff/5001/lib/compiler/implementatio...
File lib/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/11315005/diff/5001/lib/compiler/implementatio...
lib/compiler/implementation/universe/universe.dart:532: bool
hasOneSubclassThatImplements(ClassElement superClass) {
superclass is one word.

https://codereview.chromium.org/11315005/diff/5001/lib/compiler/implementatio...
lib/compiler/implementation/universe/universe.dart:536:
compiler.world.typesImplementedBySubclasses[superClass];
I'm concerned by the direct use of maps. Could you try to encapsulate this?

I actually think it would make sense to have this operation on universe.

[ngeoffray, 2012-10-29 13:35:18]

PTAL

https://codereview.chromium.org/11315005/diff/5001/lib/compiler/implementatio...
File lib/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/11315005/diff/5001/lib/compiler/implementatio...
lib/compiler/implementation/universe/universe.dart:532: bool
hasOneSubclassThatImplements(ClassElement superClass) {
On 2012/10/29 13:15:21, ahe wrote:
> superclass is one word.

Done.

https://codereview.chromium.org/11315005/diff/5001/lib/compiler/implementatio...
lib/compiler/implementation/universe/universe.dart:536:
compiler.world.typesImplementedBySubclasses[superClass];
On 2012/10/29 13:15:21, ahe wrote:
> I'm concerned by the direct use of maps. Could you try to encapsulate this?
> 
> I actually think it would make sense to have this operation on universe.

Done.

[sra1, 2012-10-29 13:46:10]

https://codereview.chromium.org/11315005/diff/9001/lib/compiler/implementatio...
File lib/compiler/implementation/world.dart (right):

https://codereview.chromium.org/11315005/diff/9001/lib/compiler/implementatio...
lib/compiler/implementation/world.dart:125: bool
hasOneSubclassThatImplements(ClassElement superClass, Type type) {
By 'One' do you mean exactly 1, or 'Any'?
Perhaps just  'hasSubclassThatImplements'

https://codereview.chromium.org/11315005/diff/9001/tests/language/typed_selec...
File tests/language/typed_selector_test.dart (right):

https://codereview.chromium.org/11315005/diff/9001/tests/language/typed_selec...
tests/language/typed_selector_test.dart:5: 
A comment explaining the issue would be helpful.

[ngeoffray, 2012-10-29 14:05:37]

https://codereview.chromium.org/11315005/diff/9001/lib/compiler/implementatio...
File lib/compiler/implementation/world.dart (right):

https://codereview.chromium.org/11315005/diff/9001/lib/compiler/implementatio...
lib/compiler/implementation/world.dart:125: bool
hasOneSubclassThatImplements(ClassElement superClass, Type type) {
On 2012/10/29 13:46:10, sra1 wrote:
> By 'One' do you mean exactly 1, or 'Any'?
> Perhaps just  'hasSubclassThatImplements'

Good point. Changed 'One' to 'Any' to make it consistent with the other methods
above.

https://codereview.chromium.org/11315005/diff/9001/tests/language/typed_selec...
File tests/language/typed_selector_test.dart (right):

https://codereview.chromium.org/11315005/diff/9001/tests/language/typed_selec...
tests/language/typed_selector_test.dart:5: 
On 2012/10/29 13:46:10, sra1 wrote:
> A comment explaining the issue would be helpful.

Done.

[ahe, 2012-10-29 14:22:14]

LGTM

https://codereview.chromium.org/11315005/diff/9001/lib/compiler/implementatio...
File lib/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/11315005/diff/9001/lib/compiler/implementatio...
lib/compiler/implementation/universe/universe.dart:531: assert(compiler.phase ==
Compiler.PHASE_COMPILING);
Perhaps this assertion should be the first line of this method.

https://codereview.chromium.org/11315005/diff/9001/lib/compiler/implementatio...
File lib/compiler/implementation/world.dart (right):

https://codereview.chromium.org/11315005/diff/9001/lib/compiler/implementatio...
lib/compiler/implementation/world.dart:124: // Returnw whether a subclass of
[superClass] implements [type].
// -> ///
Returnw -> Return
superclass is one word

https://codereview.chromium.org/11315005/diff/9001/lib/compiler/implementatio...
lib/compiler/implementation/world.dart:125: bool
hasOneSubclassThatImplements(ClassElement superClass, Type type) {
superclass is one word

[ngeoffray, 2012-10-29 14:37:53]

https://codereview.chromium.org/11315005/diff/9001/lib/compiler/implementatio...
File lib/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/11315005/diff/9001/lib/compiler/implementatio...
lib/compiler/implementation/universe/universe.dart:531: assert(compiler.phase ==
Compiler.PHASE_COMPILING);
On 2012/10/29 14:22:14, ahe wrote:
> Perhaps this assertion should be the first line of this method.

Done.

https://codereview.chromium.org/11315005/diff/9001/lib/compiler/implementatio...
File lib/compiler/implementation/world.dart (right):

https://codereview.chromium.org/11315005/diff/9001/lib/compiler/implementatio...
lib/compiler/implementation/world.dart:124: // Returnw whether a subclass of
[superClass] implements [type].
On 2012/10/29 14:22:14, ahe wrote:
> // -> ///
> Returnw -> Return
> superclass is one word

Done.

https://codereview.chromium.org/11315005/diff/9001/lib/compiler/implementatio...
lib/compiler/implementation/world.dart:125: bool
hasOneSubclassThatImplements(ClassElement superClass, Type type) {
On 2012/10/29 14:22:14, ahe wrote:
> superclass is one word

Done.