Skip to content

Expose EfficientLength interface #23328

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
DartBot opened this issue Apr 25, 2015 · 8 comments
Closed

Expose EfficientLength interface #23328

DartBot opened this issue Apr 25, 2015 · 8 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-collection type-enhancement A request for a change that isn't a bug

Comments

@DartBot
Copy link

DartBot commented Apr 25, 2015

This issue was originally filed by [email protected]


EfficientLength is currently part of dart._internal, meaning third party collection libraries can't advertise their efficientness and performance suffers as a result.

Please share ;)

@lrhn
Copy link
Member

lrhn commented Apr 27, 2015

Anything that implements List, Set, Map or Queue will also inherit EfficientLength. The classes require an efficient length operation and already implement EfficientLength.

I can see why it would be useful to mark some iterables deriving from new collections as having efficient length.
I'm sympathetic to making the marker interface public, even if it's not the best solution - and it begs for an "EfficientContains" interface too.

If exposed, the name should be changed. As an internal name, it's just a marker, but it actually extend Iterable, so a public implementation would be:

abstract class EfficientLengthIterable<T> extends Iterable<T> {
  const EfficientLengthIterable();
}

We should not change any return type to EfficientLengthIterable - that type is an implementation detail, not an API type. Methods returning iterables should still return Iterable and document in text that the length of that is efficient. That allows inefficient implementations as well, breaking only performance, not static types.


Added Area-Library, Library-Collection, Triaged labels.

@DartBot
Copy link
Author

DartBot commented Apr 27, 2015

This comment was originally written by [email protected]


Sounds good to me. Thanks.

@DartBot
Copy link
Author

DartBot commented May 6, 2015

This comment was originally written by @Fox32


This file has typo and is talking about a EfficientLengthIterableIterable: https://codereview.chromium.org/1104063002/patch/60001/70012

@lrhn
Copy link
Member

lrhn commented May 7, 2015

Doh, thanks.

@lrhn
Copy link
Member

lrhn commented Jun 1, 2015

The introduction of EfficientLengthIterable has been reverted.
It's still not the right way to solve the problem - it's a property of the object, but it's hidden away in the type, creating two types of Iterables. This breaks if you use wrapper classes, and it allows you to use EfficientLengthIterable as a type assertion (no matter how much we discourage it).

The real solution requires adding something to Iterable, and though it is harder to push through without breaking existing code, but in the long run, it's better.

@DartBot
Copy link
Author

DartBot commented Jun 1, 2015

This comment was originally written by [email protected]


So the suggestion would be to have Iterable.isEfficient? (Iterable.asList would also work).

That would solve my use case, but -- is there any way that could actually happen? A breaking change to Iterable sounds not just hard, but impossible.

Equivalent for my purposes would be to remove EfficientLengthIterable from the internal code and assume efficiency. This would cause bad effects for people passing around inefficient iterables. Personally I prefer this approach: defensive use of "toList" is a good idea anyway in my experience. (Debugging with lazy iterables gets very hard very fast).

Thanks!

@DartBot DartBot added Type-Enhancement area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-collection labels Jun 1, 2015
@davidmorgan
Copy link
Contributor

Any ideas where this might be headed, please?

@lrhn
Copy link
Member

lrhn commented Aug 26, 2015

We don't plan to expose the interface (so closing this issue).

We haven't decided either way on whether we will eventually make it possible to check if an iterable has an "efficient" length implementation (something that doesn't require iterating all the elements), but it definitely won't happen until Dart 2.0 since it changes the Iterable interface.

@lrhn lrhn closed this as completed Aug 26, 2015
@kevmoo kevmoo added type-enhancement A request for a change that isn't a bug and removed triaged labels Mar 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-collection type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

4 participants