Introduce a UnionTypeMask, currently limited to 4 types.

Introduce a UnionTypeMask, currently limited to 4 types.

R=kasperl@google.com, polux@google.com

Committed: https://code.google.com/p/dart/source/detail?r=23012

[polux, 2013-05-14 12:04:14]

LGTM for concrete_types_inferrer.dart

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
File
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart
(right):

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:855:
if (other == null) return null;
Out of curiosity, where did this happen?

[ngeoffray, 2013-05-14 12:06:21]

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
File
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart
(right):

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:855:
if (other == null) return null;
On 2013/05/14 12:04:14, polux wrote:
> Out of curiosity, where did this happen?

When running the cpa unit test, but I don't remember which exact test.

[ngeoffray, 2013-05-22 07:02:23]

The CL may have got lost in IO. Ping :-)

[kasperl, 2013-05-22 07:58:29]

LGTM.

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/types/flat_type_mask.dart
(right):

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/flat_type_mask.dart:11: class
FlatTypeMask implements TypeMask {
I think we should rename this to SimpleTypeMask (in another CL). This fits
better with the simplify method.

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/flat_type_mask.dart:174:
TypeMask union(TypeMask other_, Compiler compiler) {
I think I'd prefer calling the argument other and the local variable flatOther.

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/flat_type_mask.dart:246:
TypeMask intersection(TypeMask other_, Compiler compiler) {
other / flatOther?

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/types/type_mask.dart (right):

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/type_mask.dart:31: factory
TypeMask.unionOf(Iterable<TypeMask> masks, compiler) {
Add type for compiler argument?

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/type_mask.dart:63: bool
satisfies(ClassElement cls, Compiler compiler);
It's a little bit weird that this takes a class element, but I guess we'll be
moving in that direction soon so that all the DartType arguments will become
elements.

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart
(right):

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart:14: static
void unionHelper(Iterable<TypeMask> masks,
I'd move this below unionOf and rename it to unionOfHelper.

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart:17: for
(var mask in masks) {
Add type for mask?

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart:25: int
inListIndex = -1;
Add a comment that explains what you're trying to do here.

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart:26: bool
inserted = false;
inserted -> contained / covered / existing?

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart:39: if
(inListIndex != -1) {
This deserves a comment. The way you remove old elements from the disjoint list
isn't super obvious.

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart:67: //
Compute the common supertypes of the two types.
I'd move this comment one line down (have the newline before the comment).

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart:128: if
(disjointMasks.contains(other)) return this;
Use &&?

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart:144: if
(disjointMasks.contains(other)) return other;
Use &&?

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart:147: for
(var current in disjointMasks) {
Type current?

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart:240: bool
operator==(other) {
Maybe do an identity check first (cheap)?

[ngeoffray, 2013-05-22 10:30:05]

Thanks Kasper.

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/types/flat_type_mask.dart
(right):

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/flat_type_mask.dart:174:
TypeMask union(TypeMask other_, Compiler compiler) {
On 2013/05/22 07:58:29, kasperl wrote:
> I think I'd prefer calling the argument other and the local variable
flatOther.

Done.

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/flat_type_mask.dart:246:
TypeMask intersection(TypeMask other_, Compiler compiler) {
On 2013/05/22 07:58:29, kasperl wrote:
> other / flatOther?

Done.

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/types/type_mask.dart (right):

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/type_mask.dart:31: factory
TypeMask.unionOf(Iterable<TypeMask> masks, compiler) {
On 2013/05/22 07:58:29, kasperl wrote:
> Add type for compiler argument?

Done.

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/type_mask.dart:63: bool
satisfies(ClassElement cls, Compiler compiler);
On 2013/05/22 07:58:29, kasperl wrote:
> It's a little bit weird that this takes a class element, but I guess we'll be
> moving in that direction soon so that all the DartType arguments will become
> elements.

Yeah, I made it consistent with containsOnly, which should have also taken a
type. Can't wait for not having DartType in TypeMask anymore :-)

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart
(right):

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart:14: static
void unionHelper(Iterable<TypeMask> masks,
On 2013/05/22 07:58:29, kasperl wrote:
> I'd move this below unionOf and rename it to unionOfHelper.

Done.

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart:17: for
(var mask in masks) {
On 2013/05/22 07:58:29, kasperl wrote:
> Add type for mask?

Done.

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart:26: bool
inserted = false;
On 2013/05/22 07:58:29, kasperl wrote:
> inserted -> contained / covered / existing?

Good point, changed to 'covered'.

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart:39: if
(inListIndex != -1) {
On 2013/05/22 07:58:29, kasperl wrote:
> This deserves a comment. The way you remove old elements from the disjoint
list
> isn't super obvious.

Done.

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart:67: //
Compute the common supertypes of the two types.
On 2013/05/22 07:58:29, kasperl wrote:
> I'd move this comment one line down (have the newline before the comment).

Done.

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart:128: if
(disjointMasks.contains(other)) return this;
On 2013/05/22 07:58:29, kasperl wrote:
> Use &&?

Done.

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart:144: if
(disjointMasks.contains(other)) return other;
On 2013/05/22 07:58:29, kasperl wrote:
> Use &&?

Done.

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart:147: for
(var current in disjointMasks) {
On 2013/05/22 07:58:29, kasperl wrote:
> Type current?

Done.

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart:147: for
(var current in disjointMasks) {
On 2013/05/22 07:58:29, kasperl wrote:
> Type current?

Done.

https://codereview.chromium.org/14997006/diff/5024/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart:240: bool
operator==(other) {
On 2013/05/22 07:58:29, kasperl wrote:
> Maybe do an identity check first (cheap)?

Done.

[ngeoffray, 2013-05-22 12:43:57]

Committed patchset #3 manually as r23012 (presubmit successful).