Replace ClassHierarchy.classes with getOrderedClasses().

Replace ClassHierarchy.classes with getOrderedClasses().

R=ahe@google.com, kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=

Committed: https://github.com/dart-lang/sdk/commit/8ec1f95af620a18cf5f926febd3c7196527912c8

[Paul Berry, 2017-06-05 21:59:23]

lgtm assuming Kevin or Siggi agrees

https://codereview.chromium.org/2924713002/diff/1/pkg/kernel/lib/src/incremen...
File pkg/kernel/lib/src/incremental_class_hierarchy.dart (right):

https://codereview.chromium.org/2924713002/diff/1/pkg/kernel/lib/src/incremen...
pkg/kernel/lib/src/incremental_class_hierarchy.dart:633: /// unique identifiers
(ascending order), or `null` if the it has not
I think you mean "...if the list has not been computed yet."

[scheglov, 2017-06-05 22:01:33]

https://codereview.chromium.org/2924713002/diff/1/pkg/kernel/lib/src/incremen...
File pkg/kernel/lib/src/incremental_class_hierarchy.dart (right):

https://codereview.chromium.org/2924713002/diff/1/pkg/kernel/lib/src/incremen...
pkg/kernel/lib/src/incremental_class_hierarchy.dart:633: /// unique identifiers
(ascending order), or `null` if the it has not
On 2017/06/05 21:59:22, Paul Berry wrote:
> I think you mean "...if the list has not been computed yet."

Correct. Thank you.

[Siggi Cherem (dart-lang), 2017-06-06 00:15:21]

https://codereview.chromium.org/2924713002/diff/20001/pkg/kernel/lib/class_hi...
File pkg/kernel/lib/class_hierarchy.dart (right):

https://codereview.chromium.org/2924713002/diff/20001/pkg/kernel/lib/class_hi...
pkg/kernel/lib/class_hierarchy.dart:143: Iterable<Class>
getOrderedClasses(Iterable<Class> unordered) => classes;
mmm - It seems a bit surprising if you pass 2 classes in and the result has all
classes in the program too (not just the extra classes from supertypes).

We should at least mention it in the doc above, but I might prefer using 2 APIs
here:
  - make `getOrderedClasses` filter the result based on what was passed in (this
would not be needed in the incremental implementation, only here)
  - keep the `classes` API, but implement it on the incremental version by
forcing all classes to be created at that moment (like the eager implementation
does at construction or like you did in the transformer)

[scheglov, 2017-06-06 01:33:27]

PTAL

https://codereview.chromium.org/2924713002/diff/20001/pkg/kernel/lib/class_hi...
File pkg/kernel/lib/class_hierarchy.dart (right):

https://codereview.chromium.org/2924713002/diff/20001/pkg/kernel/lib/class_hi...
pkg/kernel/lib/class_hierarchy.dart:143: Iterable<Class>
getOrderedClasses(Iterable<Class> unordered) => classes;
On 2017/06/06 00:15:21, Siggi Cherem (dart-lang) wrote:
> mmm - It seems a bit surprising if you pass 2 classes in and the result has
all
> classes in the program too (not just the extra classes from supertypes).
> 
> We should at least mention it in the doc above, but I might prefer using 2
APIs
> here:
>   - make `getOrderedClasses` filter the result based on what was passed in
(this
> would not be needed in the incremental implementation, only here)
>   - keep the `classes` API, but implement it on the incremental version by
> forcing all classes to be created at that moment (like the eager
implementation
> does at construction or like you did in the transformer)

I updated `getOrderedClasses` to perform filtering.

The only client of `getOrderedClasses` which is not obviously whole-program
transformation is `InsertCovarianceChecks`.

All the other clients work over whole programs, and can continue using
`ClosedWorldClassHierarchy.classes`, even if it is not API of `ClassHierarchy`.

There is no such thing as "all classes" in `IncrementalClassHierarchy` - we
build only what we were asked about.

[Siggi Cherem (dart-lang), 2017-06-06 03:03:44]

lgtm.

Given that Kevin is OOO, I'm OK submitting and if Kevin has comments we can
address them as a follow up.

https://codereview.chromium.org/2924713002/diff/20001/pkg/kernel/lib/class_hi...
File pkg/kernel/lib/class_hierarchy.dart (right):

https://codereview.chromium.org/2924713002/diff/20001/pkg/kernel/lib/class_hi...
pkg/kernel/lib/class_hierarchy.dart:143: Iterable<Class>
getOrderedClasses(Iterable<Class> unordered) => classes;
On 2017/06/06 01:33:27, scheglov wrote:
> On 2017/06/06 00:15:21, Siggi Cherem (dart-lang) wrote:
> > mmm - It seems a bit surprising if you pass 2 classes in and the result has
> all
> > classes in the program too (not just the extra classes from supertypes).
> > 
> > We should at least mention it in the doc above, but I might prefer using 2
> APIs
> > here:
> >   - make `getOrderedClasses` filter the result based on what was passed in
> (this
> > would not be needed in the incremental implementation, only here)
> >   - keep the `classes` API, but implement it on the incremental version by
> > forcing all classes to be created at that moment (like the eager
> implementation
> > does at construction or like you did in the transformer)
> 
> I updated `getOrderedClasses` to perform filtering.
> 
> The only client of `getOrderedClasses` which is not obviously whole-program
> transformation is `InsertCovarianceChecks`.
> 
> All the other clients work over whole programs, and can continue using
> `ClosedWorldClassHierarchy.classes`, even if it is not API of
`ClassHierarchy`.
> 
> There is no such thing as "all classes" in `IncrementalClassHierarchy` - we
> build only what we were asked about.

sorry, makes sense,  I forgot that  IncrementalClassHierarchy never takes a
program

[scheglov, 2017-06-06 03:23:19]

Description was changed from

==========
Replace ClassHierarchy.classes with getOrderedClasses().

R=ahe@google.com, kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=
==========

to

==========
Replace ClassHierarchy.classes with getOrderedClasses().

R=ahe@google.com, kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=

Committed:
https://github.com/dart-lang/sdk/commit/8ec1f95af620a18cf5f926febd3c7196527912c8
==========

[scheglov, 2017-06-06 03:23:20]

Committed patchset #3 (id:40001) manually as
8ec1f95af620a18cf5f926febd3c7196527912c8 (presubmit successful).

[ahe, 2017-06-06 11:56:15]

lgtm