fix ListMixin _filter and sort to work in strong mode

fix ListMixin _filter and sort to work in strong mode

R=lrn@google.com

Committed: https://github.com/dart-lang/sdk/commit/373149e00390ad58e201c3bf975e8312c3613d8c

[Jennifer Messerly, 2017-02-24 23:23:57]

jmesserly@google.com changed reviewers:
+ floitsch@google.com, lrn@google.com

[Jennifer Messerly, 2017-02-24 23:23:57]

Here's one more strong mode fix to the SDK.

https://codereview.chromium.org/2715833004/diff/1/sdk/lib/collection/list.dart
File sdk/lib/collection/list.dart (right):

https://codereview.chromium.org/2715833004/diff/1/sdk/lib/collection/list.dar...
sdk/lib/collection/list.dart:278: List<E> retained = <E>[];
This change is needed to prevent a cast failure in strong mode when we cast to
List<E>

https://codereview.chromium.org/2715833004/diff/1/sdk/lib/collection/list.dar...
sdk/lib/collection/list.dart:310: Sort.sort(this, (a, b) =>
Comparable.compare(a, b));
This change is because Comparable.compare requires that the arguments ("a" and
"b") are Comparables, which is not guaranteed statically for a ListMixin<E>.
This will require a runtime downcast here for each item in the list.

[Lasse Reichstein Nielsen, 2017-02-25 10:53:15]

lgtm

https://codereview.chromium.org/2715833004/diff/1/sdk/lib/collection/list.dart
File sdk/lib/collection/list.dart (right):

https://codereview.chromium.org/2715833004/diff/1/sdk/lib/collection/list.dar...
sdk/lib/collection/list.dart:278: List<E> retained = <E>[];
On 2017/02/24 23:23:57, Jennifer Messerly wrote:
> This change is needed to prevent a cast failure in strong mode when we cast to
> List<E>
> 
> 

Acknowledged.

https://codereview.chromium.org/2715833004/diff/1/sdk/lib/collection/list.dar...
sdk/lib/collection/list.dart:310: Sort.sort(this, (a, b) =>
Comparable.compare(a, b));
Comparable.compare was always a type hack, so it's not surprising that it's
failing in strong mode.
We can't check that `this is List<Comparable>` because we do want to sort a
List<Object> of ints ... so I guess this is as good as it gets.

I also don't want to change Comparable.compare to take (dynamic,dynamic) as
argument, that's probably too expensive (making every compareTo call dynamic).

Consider adding a static `int _compareAny(a, b) => a.compareTo(b);` and use that
instead of wrapping Comparable.compare for each invocation.

[Jennifer Messerly, 2017-02-27 18:26:24]

Patchset #2 (id:20001) has been deleted

[Jennifer Messerly, 2017-02-27 18:28:08]

Thanks! I've added _compareAny if you want to take another look.

https://codereview.chromium.org/2715833004/diff/1/sdk/lib/collection/list.dart
File sdk/lib/collection/list.dart (right):

https://codereview.chromium.org/2715833004/diff/1/sdk/lib/collection/list.dar...
sdk/lib/collection/list.dart:310: Sort.sort(this, (a, b) =>
Comparable.compare(a, b));
On 2017/02/25 10:53:15, Lasse Reichstein Nielsen wrote:
> Comparable.compare was always a type hack, so it's not surprising that it's
> failing in strong mode.
> We can't check that `this is List<Comparable>` because we do want to sort a
> List<Object> of ints ... so I guess this is as good as it gets.
> 
> I also don't want to change Comparable.compare to take (dynamic,dynamic) as
> argument, that's probably too expensive (making every compareTo call dynamic).
> 
> Consider adding a static `int _compareAny(a, b) => a.compareTo(b);` and use
that
> instead of wrapping Comparable.compare for each invocation.

good catch, added _compareAny

[Lasse Reichstein Nielsen, 2017-02-28 07:05:57]

lgtm

https://codereview.chromium.org/2715833004/diff/40001/sdk/lib/collection/list...
File sdk/lib/collection/list.dart (right):

https://codereview.chromium.org/2715833004/diff/40001/sdk/lib/collection/list...
sdk/lib/collection/list.dart:313: // In strong mode this requires a cast to
ensure `a` and `b` are Comparable.
Confusing comment - I don't see any cast?

[Jennifer Messerly, 2017-02-28 18:36:35]

thank you!

https://codereview.chromium.org/2715833004/diff/40001/sdk/lib/collection/list...
File sdk/lib/collection/list.dart (right):

https://codereview.chromium.org/2715833004/diff/40001/sdk/lib/collection/list...
sdk/lib/collection/list.dart:277: void _filter(List source, bool test(var
element), bool retainMatching) {
I also made the change you suggested here:
https://codereview.chromium.org/2698353003/diff/80001/sdk/lib/collection/list...

https://codereview.chromium.org/2715833004/diff/40001/sdk/lib/collection/list...
sdk/lib/collection/list.dart:313: // In strong mode this requires a cast to
ensure `a` and `b` are Comparable.
On 2017/02/28 07:05:56, Lasse Reichstein Nielsen wrote:
> Confusing comment - I don't see any cast?

Ah thanks! will clarify. The cast is added implicitly.

[Jennifer Messerly, 2017-02-28 18:37:36]

Description was changed from

==========
fix ListMixin _filter and sort to work in strong mode
==========

to

==========
fix ListMixin _filter and sort to work in strong mode

R=lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/373149e00390ad58e201c3bf975e8312c3613d8c
==========

[Jennifer Messerly, 2017-02-28 18:37:38]

Committed patchset #4 (id:80001) manually as
373149e00390ad58e201c3bf975e8312c3613d8c (presubmit successful).