Add `is_empty` function to `ExactSizeIterator` #35428

Open
frewsxcv opened this Issue Aug 6, 2016 · 50 comments

Comments

Projects
None yet
@frewsxcv
Member

frewsxcv commented Aug 6, 2016

Tracking issue for functionality introduced in #34357

@frewsxcv

This comment has been minimized.

Show comment
Hide comment
@frewsxcv

frewsxcv Aug 6, 2016

Member

Added reference to this issue in #35429

Member

frewsxcv commented Aug 6, 2016

Added reference to this issue in #35429

@apasel422 apasel422 added the B-unstable label Aug 6, 2016

jonathandturner added a commit to jonathandturner/rust that referenced this issue Aug 7, 2016

Rollup merge of #35429 - frewsxcv:tracking-is-empty, r=apasel422
Indicate tracking issue for `exact_size_is_empty` unstability.

rust-lang#35428

@alexcrichton alexcrichton added the T-libs label Aug 7, 2016

jonathandturner added a commit to jonathandturner/rust that referenced this issue Aug 7, 2016

Rollup merge of #35429 - frewsxcv:tracking-is-empty, r=apasel422
Indicate tracking issue for `exact_size_is_empty` unstability.

rust-lang#35428

jonathandturner added a commit to jonathandturner/rust that referenced this issue Aug 7, 2016

Rollup merge of #35429 - frewsxcv:tracking-is-empty, r=apasel422
Indicate tracking issue for `exact_size_is_empty` unstability.

rust-lang#35428

@mcarton mcarton referenced this issue in rust-lang-nursery/rust-clippy Aug 29, 2016

Closed

`len_without_is_empty` in private types #1085

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Oct 17, 2016

Contributor

I wanted to use this today until I realized that it’s unstable. (I could use .len() == 0 in the mean time.) Seems straightforward enough. FCP to stabilize?

Contributor

SimonSapin commented Oct 17, 2016

I wanted to use this today until I realized that it’s unstable. (I could use .len() == 0 in the mean time.) Seems straightforward enough. FCP to stabilize?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Nov 1, 2016

Member

@rfcbot fcp merge

Seems good to have consistency!

Member

alexcrichton commented Nov 1, 2016

@rfcbot fcp merge

Seems good to have consistency!

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Nov 1, 2016

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented Nov 1, 2016

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Nov 4, 2016

Contributor

@SimonSapin I'm curious, when do you use ExactSizeIterator and have the opportunity to use this?

Contributor

bluss commented Nov 4, 2016

@SimonSapin I'm curious, when do you use ExactSizeIterator and have the opportunity to use this?

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Nov 5, 2016

Contributor

When using a slice or vec iterator as input for a parser, .is_empty() is a way to tell "have I reach the end of the input yet?" https://github.com/servo/html5ever/blob/b5c4552fab/macros/match_token.rs#L175

Contributor

SimonSapin commented Nov 5, 2016

When using a slice or vec iterator as input for a parser, .is_empty() is a way to tell "have I reach the end of the input yet?" https://github.com/servo/html5ever/blob/b5c4552fab/macros/match_token.rs#L175

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Nov 5, 2016

Contributor

Aha, that explains it for me: using a specific iterator type, not using ESI generically. This method is good, reinforces existing conventions.

Contributor

bluss commented Nov 5, 2016

Aha, that explains it for me: using a specific iterator type, not using ESI generically. This method is good, reinforces existing conventions.

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Nov 12, 2016

🔔 This is now entering its final comment period, as per the review above. 🔔

psst @alexcrichton, I wasn't able to add the final-comment-period label, please do so.

rfcbot commented Nov 12, 2016

🔔 This is now entering its final comment period, as per the review above. 🔔

psst @alexcrichton, I wasn't able to add the final-comment-period label, please do so.

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Nov 22, 2016

The final comment period is now complete.

rfcbot commented Nov 22, 2016

The final comment period is now complete.

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Nov 22, 2016

Contributor

used in PR #37943

Contributor

bluss commented Nov 22, 2016

used in PR #37943

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Nov 23, 2016

Contributor

Here's a thought: Some iterators can implement a good is_empty() even if they can't do ESI or .len(). One example is .chars(). Does it matter?

Contributor

bluss commented Nov 23, 2016

Here's a thought: Some iterators can implement a good is_empty() even if they can't do ESI or .len(). One example is .chars(). Does it matter?

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Dec 15, 2016

Member

@bluss Sorry I missed your comment when prepping the stabilization PR.

My feeling is that these iterators can/should provide an inherent is_empty method if we think that's useful. The addition of the method to ExactSizeIterator is more about convenience than generic programming, IMO.

FWIW, it's also a longstanding desire on the lang side to have an API evolution path for splitting a trait into supertraits without breaking existing code. It's not guaranteed to happen, but that would allow us to split out is_empty into its own trait if we wanted to program generically with it in std in the future. (Of course, we can always add a separate trait with a blanket impl, worst case.)

Member

aturon commented Dec 15, 2016

@bluss Sorry I missed your comment when prepping the stabilization PR.

My feeling is that these iterators can/should provide an inherent is_empty method if we think that's useful. The addition of the method to ExactSizeIterator is more about convenience than generic programming, IMO.

FWIW, it's also a longstanding desire on the lang side to have an API evolution path for splitting a trait into supertraits without breaking existing code. It's not guaranteed to happen, but that would allow us to split out is_empty into its own trait if we wanted to program generically with it in std in the future. (Of course, we can always add a separate trait with a blanket impl, worst case.)

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Dec 15, 2016

Member

I'm removing is_empty from the current stabilization PR, to give more time for discussion here.

Member

aturon commented Dec 15, 2016

I'm removing is_empty from the current stabilization PR, to give more time for discussion here.

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Dec 15, 2016

Contributor
  • I think emptiness should be a trait, so that it can pass through adapters
  • Iterators as lazy sequences have a different relationship to emptiness than collections, and the common chars() can already tell emptiness but not length; it's a common case in for example parsers (maybe not the most common, since byte-level parsing seems to be the most popular).
Contributor

bluss commented Dec 15, 2016

  • I think emptiness should be a trait, so that it can pass through adapters
  • Iterators as lazy sequences have a different relationship to emptiness than collections, and the common chars() can already tell emptiness but not length; it's a common case in for example parsers (maybe not the most common, since byte-level parsing seems to be the most popular).
@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Dec 15, 2016

Member

@bluss

Both points make sense. However, without further language improvements, it will be tricky to meet those goals and the original ergonomic goal at the same time.

Ideally, ExactSizeIterator would be a subtrait of IsEmpty and provide a default implementation of the is_empty method. To make this work, we'll need both specialization and the ability to implicitly impl supertraits when impl'ing a subtrait (an idea we've been kicking around on the lang team precisely to allow for this kind of API evolution).

Alternatively, we could add the IsEmpty trait to the prelude, along with a blanket impl from ExactSizeIterator. That comes with its own backwards-compatibility risks, though.

Member

aturon commented Dec 15, 2016

@bluss

Both points make sense. However, without further language improvements, it will be tricky to meet those goals and the original ergonomic goal at the same time.

Ideally, ExactSizeIterator would be a subtrait of IsEmpty and provide a default implementation of the is_empty method. To make this work, we'll need both specialization and the ability to implicitly impl supertraits when impl'ing a subtrait (an idea we've been kicking around on the lang team precisely to allow for this kind of API evolution).

Alternatively, we could add the IsEmpty trait to the prelude, along with a blanket impl from ExactSizeIterator. That comes with its own backwards-compatibility risks, though.

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Dec 15, 2016

Contributor

don't we have a backdoor for adding methods like this? trait Iterator has the method .rposition() where Self: DoubleEndedIterator

Contributor

bluss commented Dec 15, 2016

don't we have a backdoor for adding methods like this? trait Iterator has the method .rposition() where Self: DoubleEndedIterator

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Dec 15, 2016

Member

@bluss I may be missing something, but I don't see how that helps here. To clarify, the competing goals I see are:

  • A separate IsEmpty trait that can be properly forwarded along adapters, and programmed over generically.
  • Ergonomic access to is_empty for any ExactSizeIterator.

I don't offhand see how the trick you mention helps here; maybe you can spell it out?

It's worth noting that we could provide an is_empty method both in ExactSizeIterator and in a separate IsEmpty trait, with a blanket impl as well. But of course if you have both traits in scope at the same time, you'll have to explicitly disambiguate.

Member

aturon commented Dec 15, 2016

@bluss I may be missing something, but I don't see how that helps here. To clarify, the competing goals I see are:

  • A separate IsEmpty trait that can be properly forwarded along adapters, and programmed over generically.
  • Ergonomic access to is_empty for any ExactSizeIterator.

I don't offhand see how the trick you mention helps here; maybe you can spell it out?

It's worth noting that we could provide an is_empty method both in ExactSizeIterator and in a separate IsEmpty trait, with a blanket impl as well. But of course if you have both traits in scope at the same time, you'll have to explicitly disambiguate.

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Dec 15, 2016

Contributor

It's easier said than done, apparently. trait Iterator can get a new method, something like:

fn is_empty(&self) -> bool
    where Self: IsEmptyIterator;

which fixes the issue with having the method in the prelude.

But to arrange the right blanket implementations for IsEmptyIterator does not seem to be possible, even with the specialization features that are in nightly now.

Contributor

bluss commented Dec 15, 2016

It's easier said than done, apparently. trait Iterator can get a new method, something like:

fn is_empty(&self) -> bool
    where Self: IsEmptyIterator;

which fixes the issue with having the method in the prelude.

But to arrange the right blanket implementations for IsEmptyIterator does not seem to be possible, even with the specialization features that are in nightly now.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Mar 9, 2017

Contributor

Did the IsEmpty trait preempt the stabilization FCP?

Contributor

SimonSapin commented Mar 9, 2017

Did the IsEmpty trait preempt the stabilization FCP?

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Mar 13, 2017

Member

@SimonSapin Yes, this ended up getting pulled from the stabilization PR and has been sitting idle since then. Needs someone to drive it to a conclusion.

Member

aturon commented Mar 13, 2017

@SimonSapin Yes, this ended up getting pulled from the stabilization PR and has been sitting idle since then. Needs someone to drive it to a conclusion.

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Mar 13, 2017

Contributor

I'd like to modify ESI to be something like this:

trait ExactSizeIterator : IsEmpty {
    fn len(&self) -> usize;
}

trait IsEmpty {
    fn is_empty(&self) -> bool;
}

impl<T> IsEmpty for T where T: ExactSizeIterator {
    default fn is_empty(&self) -> bool {
        self.len() == 0
    }
}

specialization seems to implement this correctly, so I don't immediately see any problems with it. It doesn't resolve having the method in the prelude.

Iterator could get a new method:

pub trait Iterator {
    ...
    fn is_empty(&self) -> bool
        where Self: IsEmpty
    {
        IsEmpty::is_empty(self)
    }
}

which puts it in the prelude.

Contributor

bluss commented Mar 13, 2017

I'd like to modify ESI to be something like this:

trait ExactSizeIterator : IsEmpty {
    fn len(&self) -> usize;
}

trait IsEmpty {
    fn is_empty(&self) -> bool;
}

impl<T> IsEmpty for T where T: ExactSizeIterator {
    default fn is_empty(&self) -> bool {
        self.len() == 0
    }
}

specialization seems to implement this correctly, so I don't immediately see any problems with it. It doesn't resolve having the method in the prelude.

Iterator could get a new method:

pub trait Iterator {
    ...
    fn is_empty(&self) -> bool
        where Self: IsEmpty
    {
        IsEmpty::is_empty(self)
    }
}

which puts it in the prelude.

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Mar 13, 2017

Member

cc @rust-lang/libs, thoughts on @bluss's proposed change?

The overall motivation here is to allow for greater customization of is_empty, and to pass it through iterator adapters and the like.

Note that customization would currently only be allowed on nightly via specialization.

Member

aturon commented Mar 13, 2017

cc @rust-lang/libs, thoughts on @bluss's proposed change?

The overall motivation here is to allow for greater customization of is_empty, and to pass it through iterator adapters and the like.

Note that customization would currently only be allowed on nightly via specialization.

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Mar 13, 2017

Contributor

And so that chars() has the same is_empty even though it's not an ESI.

Contributor

bluss commented Mar 13, 2017

And so that chars() has the same is_empty even though it's not an ESI.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Mar 14, 2017

Member

Given that is_empty was basically purely added for our len convention (where if you have len you have is_empty) I feel like blowing that up into an entire trait with specialization and new methods is way overkill.

If we can't stabilize it as is I would prefer to remove it as it doesn't seem to have strong enough motivation for a new trait and combinator.

Member

alexcrichton commented Mar 14, 2017

Given that is_empty was basically purely added for our len convention (where if you have len you have is_empty) I feel like blowing that up into an entire trait with specialization and new methods is way overkill.

If we can't stabilize it as is I would prefer to remove it as it doesn't seem to have strong enough motivation for a new trait and combinator.

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Mar 14, 2017

Contributor

Right, I don't think it's right to add an is_empty only for ESI, when it makes sense for a much larger class of iterators. I could see is_empty coming back in a new trait with an RFC, though.

@SimonSapin Thoughts? I thought you might weigh in on chars() and related iterators, if they should have is_empty() too.

Contributor

bluss commented Mar 14, 2017

Right, I don't think it's right to add an is_empty only for ESI, when it makes sense for a much larger class of iterators. I could see is_empty coming back in a new trait with an RFC, though.

@SimonSapin Thoughts? I thought you might weigh in on chars() and related iterators, if they should have is_empty() too.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Mar 14, 2017

Contributor

An IsEmpty trait also seems slightly overkill to me, but I’m ok with it if other people want it.

I think it’s also fine to not have a dedicated trait but only a default method on ExactSizeIterator and an inherent method on str::Chars (and others as appropriate). Note that the latter is already possible through .as_str().is_empty().

Contributor

SimonSapin commented Mar 14, 2017

An IsEmpty trait also seems slightly overkill to me, but I’m ok with it if other people want it.

I think it’s also fine to not have a dedicated trait but only a default method on ExactSizeIterator and an inherent method on str::Chars (and others as appropriate). Note that the latter is already possible through .as_str().is_empty().

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Apr 9, 2017

Contributor

Personally I think that is_empty as a convenience method here is useful; I've found myself directly checking is_empty on iterators sometimes and removing this method would be a bit of a pain.

Also, on the note of generic traits for is_empty, I've been slowly working on my len-trait crate and that may be of use to you, @bluss. In the future it may be worth having some sort of generalisation over these things in stdlib, but atm I'd honestly rather just have is_empty on ExactSizeIterator and deal with having the method duplicated over multiple traits if it comes down to it.

Contributor

clarcharr commented Apr 9, 2017

Personally I think that is_empty as a convenience method here is useful; I've found myself directly checking is_empty on iterators sometimes and removing this method would be a bit of a pain.

Also, on the note of generic traits for is_empty, I've been slowly working on my len-trait crate and that may be of use to you, @bluss. In the future it may be worth having some sort of generalisation over these things in stdlib, but atm I'd honestly rather just have is_empty on ExactSizeIterator and deal with having the method duplicated over multiple traits if it comes down to it.

@nvzqz

This comment has been minimized.

Show comment
Hide comment
@nvzqz

nvzqz May 4, 2017

Contributor

I'm very much in favor of a separate IsEmpty trait as described by @bluss.

One solid use case of is_empty() is in a chess engine. They commonly use a Bitboard type that can be implemented as an exact size iterator of 64 bits that map to squares. len() would be implemented via count_ones() on a u64. However, is_empty() can be made faster by simply checking if the internal u64 is zero rather than checking if len() returns zero.

Contributor

nvzqz commented May 4, 2017

I'm very much in favor of a separate IsEmpty trait as described by @bluss.

One solid use case of is_empty() is in a chess engine. They commonly use a Bitboard type that can be implemented as an exact size iterator of 64 bits that map to squares. len() would be implemented via count_ones() on a u64. However, is_empty() can be made faster by simply checking if the internal u64 is zero rather than checking if len() returns zero.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin May 11, 2017

Contributor

@nvzqz Is this chess board type used in a generic context? If not you can make an inherent is_empty method, no need for a standard library trait.

Contributor

SimonSapin commented May 11, 2017

@nvzqz Is this chess board type used in a generic context? If not you can make an inherent is_empty method, no need for a standard library trait.

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr May 11, 2017

Contributor

IMHO is_empty cases are much better covered by peek than any generic trait. I'd rather just have this trait have an is_empty method.

If people want to have more generality they can use the len-trait crate I mentioned earlier. Based upon this discussion I did split out Empty and Len in that crate because it's for more niche cases.

Contributor

clarcharr commented May 11, 2017

IMHO is_empty cases are much better covered by peek than any generic trait. I'd rather just have this trait have an is_empty method.

If people want to have more generality they can use the len-trait crate I mentioned earlier. Based upon this discussion I did split out Empty and Len in that crate because it's for more niche cases.

@nvzqz

This comment has been minimized.

Show comment
Hide comment
@nvzqz

nvzqz May 11, 2017

Contributor

@SimonSapin While it currently isn't used internally in a generic context, I would like it so that an outside user can use the Bitboard as a generic iterator with an is_empty() method.

Contributor

nvzqz commented May 11, 2017

@SimonSapin While it currently isn't used internally in a generic context, I would like it so that an outside user can use the Bitboard as a generic iterator with an is_empty() method.

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr May 12, 2017

Contributor

As I said, you can easily implement is_empty for any iterator with peekable().peek().is_none()

Contributor

clarcharr commented May 12, 2017

As I said, you can easily implement is_empty for any iterator with peekable().peek().is_none()

@ranma42

This comment has been minimized.

Show comment
Hide comment
@ranma42

ranma42 May 12, 2017

Contributor

@clarcharr peek can cause side-effects (as suggested by mut), while len and is_empty are pure.

Contributor

ranma42 commented May 12, 2017

@clarcharr peek can cause side-effects (as suggested by mut), while len and is_empty are pure.

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr May 12, 2017

Contributor

@ranma42 huh, I never realised. that seems like a bug to me

Contributor

clarcharr commented May 12, 2017

@ranma42 huh, I never realised. that seems like a bug to me

@nvzqz

This comment has been minimized.

Show comment
Hide comment
@nvzqz

nvzqz May 12, 2017

Contributor

Does peek depend on next? If so, it makes sense to me for it to require mut.

Contributor

nvzqz commented May 12, 2017

Does peek depend on next? If so, it makes sense to me for it to require mut.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin May 12, 2017

Contributor

peek is a method of Peekable, an iterator wrapper that holds an Option<Self::Item> buffer. It does call next to fill that buffer if not filled already.

Contributor

SimonSapin commented May 12, 2017

peek is a method of Peekable, an iterator wrapper that holds an Option<Self::Item> buffer. It does call next to fill that buffer if not filled already.

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr May 13, 2017

Contributor

I mean, you could easily make peek immutable:

fn peekable(self) -> Peekable<Self> {
    Peekable {
        buffer: self.next(),
        iter: self,
    }
}

and

fn peek(&self) -> Option<&Self::Item> {
    self.buffer.as_ref()
}
fn next(&mut self) -> Option<Self::Item> {
    mem::replace(self.buffer, self.next())
}

Although I guess that this is a bit out of the scope of the general discussion.

My main point is that it's almost trivial to determine if an iterator is empty, and doesn't need a generic trait. We can deal with ExactSizeIterator just having an is_empty convenience method.

Contributor

clarcharr commented May 13, 2017

I mean, you could easily make peek immutable:

fn peekable(self) -> Peekable<Self> {
    Peekable {
        buffer: self.next(),
        iter: self,
    }
}

and

fn peek(&self) -> Option<&Self::Item> {
    self.buffer.as_ref()
}
fn next(&mut self) -> Option<Self::Item> {
    mem::replace(self.buffer, self.next())
}

Although I guess that this is a bit out of the scope of the general discussion.

My main point is that it's almost trivial to determine if an iterator is empty, and doesn't need a generic trait. We can deal with ExactSizeIterator just having an is_empty convenience method.

@nvzqz

This comment has been minimized.

Show comment
Hide comment
@nvzqz

nvzqz May 13, 2017

Contributor

While the Peekable solution can serve as a trivial default, I still think that any default should be able to be overridden with a possibly faster form like with my previous example where the internal u64 is 0.

Contributor

nvzqz commented May 13, 2017

While the Peekable solution can serve as a trivial default, I still think that any default should be able to be overridden with a possibly faster form like with my previous example where the internal u64 is 0.

@scottmcm

This comment has been minimized.

Show comment
Hide comment
@scottmcm

scottmcm May 24, 2017

Member

One thing that's unfortunate about is_empty being on ExactSizeIterator is that it would also make sense on TrustedLen. And their slightly-different requirements mean that either choice leaves out things that could have it easily -- Chain xor Skip can have it, if it's on only one of those two traits. And if it's on both, it'll be ambiguous on a whole bunch of common iterators...

Member

scottmcm commented May 24, 2017

One thing that's unfortunate about is_empty being on ExactSizeIterator is that it would also make sense on TrustedLen. And their slightly-different requirements mean that either choice leaves out things that could have it easily -- Chain xor Skip can have it, if it's on only one of those two traits. And if it's on both, it'll be ambiguous on a whole bunch of common iterators...

@scottmcm

This comment has been minimized.

Show comment
Hide comment
@scottmcm

scottmcm Feb 20, 2018

Member

Range::is_empty (#48111) is another example of wanting the method on non-ESI.

Member

scottmcm commented Feb 20, 2018

Range::is_empty (#48111) is another example of wanting the method on non-ESI.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Mar 28, 2018

Contributor

The libs team discussed this and the consensus was to stabilize ExactSizeIterator as-is.

Iterator types that are not ExactSizeIterator but are still able to implement a meaningful is_empty method can do so in an inherent method. Code that needs to be generic over such types can define a non-standard-library trait.

@rfcbot fcp merge

Contributor

SimonSapin commented Mar 28, 2018

The libs team discussed this and the consensus was to stabilize ExactSizeIterator as-is.

Iterator types that are not ExactSizeIterator but are still able to implement a meaningful is_empty method can do so in an inherent method. Code that needs to be generic over such types can define a non-standard-library trait.

@rfcbot fcp merge

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Mar 28, 2018

Contributor

Oops, it looks like rfcbot is not responding because a FCP was already completed in 2016. Anyone please make a stabilization PR, we’ll FCP there.

Contributor

SimonSapin commented Mar 28, 2018

Oops, it looks like rfcbot is not responding because a FCP was already completed in 2016. Anyone please make a stabilization PR, we’ll FCP there.

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Mar 28, 2018

Contributor

(is the rfcbot code in a repo somewhere? because I'd be willing to take a look at that)

Contributor

clarcharr commented Mar 28, 2018

(is the rfcbot code in a repo somewhere? because I'd be willing to take a look at that)

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Mar 28, 2018

Contributor

As far as a proper method goes, I think that perhaps an is_empty method could also be added to FusedIterator, and is_empty could be added to Fuse. Although I think the former is already stable, so, I'm not sure if we can do that...

Contributor

clarcharr commented Mar 28, 2018

As far as a proper method goes, I think that perhaps an is_empty method could also be added to FusedIterator, and is_empty could be added to Fuse. Although I think the former is already stable, so, I'm not sure if we can do that...

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
Contributor

SimonSapin commented Mar 28, 2018

@scottmcm

This comment has been minimized.

Show comment
Hide comment
@scottmcm

scottmcm Mar 29, 2018

Member

It still seems weird to have is_empty only for things that meet the "here to make rposition work" rules of ExactSizeIterator. There are so many things for which it's obvious that it could exist, like chain, but it won't. Having it here gets literally nothing over .len() == 0, which isn't even shorter than .is_empty(). And making a third-party trait for it would be painful at best, since ExactSizeIterator is in the prelude and thus trying to call it would be ambiguous with no nice workaround.

Member

scottmcm commented Mar 29, 2018

It still seems weird to have is_empty only for things that meet the "here to make rposition work" rules of ExactSizeIterator. There are so many things for which it's obvious that it could exist, like chain, but it won't. Having it here gets literally nothing over .len() == 0, which isn't even shorter than .is_empty(). And making a third-party trait for it would be painful at best, since ExactSizeIterator is in the prelude and thus trying to call it would be ambiguous with no nice workaround.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Mar 31, 2018

Contributor

I can’t tell whether you’re arguing that is_empty is so important that it should have a dedicated trait, or that it’s unimportant enough that it doesn’t need to exist at all. We’re not adding a dedicated trait right now, but it or inherent methods on other iterators can still be proposed in a separate RFC or PR. As to .len() == 0, I think it’s less about character count than about clarifying intent. Collections (slices, maps) already have an is_empty method.

Contributor

SimonSapin commented Mar 31, 2018

I can’t tell whether you’re arguing that is_empty is so important that it should have a dedicated trait, or that it’s unimportant enough that it doesn’t need to exist at all. We’re not adding a dedicated trait right now, but it or inherent methods on other iterators can still be proposed in a separate RFC or PR. As to .len() == 0, I think it’s less about character count than about clarifying intent. Collections (slices, maps) already have an is_empty method.

@nvzqz

This comment has been minimized.

Show comment
Hide comment
@nvzqz

nvzqz Mar 31, 2018

Contributor

On top of being able to express intent with is_empty, it may be more optimal to call than .len() == 0. One case that comes to mind is a linked list iterator where is_empty is a constant time operation whereas len may take linear time.

I don't think is_empty should go on ExactSizeIterator, however. There may be iterators where the remaining number of elements is unknown, but it is known whether or not there are more elements left.

Contributor

nvzqz commented Mar 31, 2018

On top of being able to express intent with is_empty, it may be more optimal to call than .len() == 0. One case that comes to mind is a linked list iterator where is_empty is a constant time operation whereas len may take linear time.

I don't think is_empty should go on ExactSizeIterator, however. There may be iterators where the remaining number of elements is unknown, but it is known whether or not there are more elements left.

@scottmcm

This comment has been minimized.

Show comment
Hide comment
@scottmcm

scottmcm Apr 1, 2018

Member

@SimonSapin I'm arguing that is_empty is too useful to be restricted to only things (particularly adapters) that are ExactSizedIterator. (I agree that the clearer intent is valuable.) And I don't think "a dedicated trait" can be usefully added after this is stabilized, since it'd cause name conflicts with this one. I'd rather live with inherent methods and only-a-tiny-bit-worse .len() == 0 for now to keep from closing off a straight-forward path to having .is_empty() on things like Chain.

I don't think inherent methods are a great option for is_empty() either, since they disappear as soon as you .map() them. And there's no reason Map<RangeInclusive<usize>, _>, say, shouldn't have is_empty.

Member

scottmcm commented Apr 1, 2018

@SimonSapin I'm arguing that is_empty is too useful to be restricted to only things (particularly adapters) that are ExactSizedIterator. (I agree that the clearer intent is valuable.) And I don't think "a dedicated trait" can be usefully added after this is stabilized, since it'd cause name conflicts with this one. I'd rather live with inherent methods and only-a-tiny-bit-worse .len() == 0 for now to keep from closing off a straight-forward path to having .is_empty() on things like Chain.

I don't think inherent methods are a great option for is_empty() either, since they disappear as soon as you .map() them. And there's no reason Map<RangeInclusive<usize>, _>, say, shouldn't have is_empty.

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Apr 1, 2018

Contributor

I think that putting it on FusedIterator would be best but that would require un-stabilising it after it's been in beta, when a release is around the corner.

Contributor

clarcharr commented Apr 1, 2018

I think that putting it on FusedIterator would be best but that would require un-stabilising it after it's been in beta, when a release is around the corner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment