Skip to content

ExactSize: Reversible enumerate and generic rposition() #8884

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
wants to merge 9 commits into from
Closed

ExactSize: Reversible enumerate and generic rposition() #8884

wants to merge 9 commits into from

Conversation

bluss
Copy link
Member

@bluss bluss commented Aug 30, 2013

The message of the first commit explains (edited for changed trait name):

The trait ExactSize is introduced to solve a few small niggles:

  • We can't reverse (.invert()) an enumeration iterator
  • for a vector, we have v.iter().position(f) but v.rposition(f).
  • We can't reverse Zip even if both iterators are from vectors

ExactSize is an empty trait that is intended to indicate that an
iterator, for example VecIterator, knows its exact finite size and
reports it correctly using .size_hint(). Only adaptors that preserve
this at all times, can expose this trait further. (Where here we say
finite for fitting in uint).


It may seem complicated just to solve these small "niggles",
(It's really the reversible enumerate case that's the most interesting)
but only a few core iterators need to implement this trait.

While we gain more capabilities generically for some iterators,
it becomes a tad more complicated to figure out if a type has
the right trait impls for it.

blake2-ppc added 6 commits August 30, 2013 20:03
The trait `ExactSizeHint` is introduced to solve a few small niggles:

* We can't reverse (`.invert()`) an enumeration iterator
* for a vector, we have `v.iter().position(f)` but `v.rposition(f)`.
* We can't reverse `Zip` even if both iterators are from vectors

`ExactSizeHint` is an empty trait that is intended to indicate that an
iterator, for example `VecIterator`, knows its exact finite size and
reports it correctly using `.size_hint()`. Only adaptors that preserve
this at all times, can expose this trait further. (Where here we say
finite for fitting in uint).
Let Zip be double-ended when both its children have the ExactSizeHint
trait.
This is a generalization of the vector .rposition() method, to all
double-ended iterators that have the ExactSizeHint trait.

This resolves the slight asymmetry around `position` and `rposition`

* position from front is `vec.iter().position()`
* position from the back was, `vec.rposition()` is now `vec.iter().rposition()`

Additionally, other indexed sequences (only `extra::ringbuf` I think),
will have the same method available once it implements ExactSizeHint.
Simplify code by using the reversibility of enumerate and use
.rposition().
Caught a bug where .enumerate() was used on a reverse iterator. The
indices should be counted from the front here (bblum confirms).
@huonw
Copy link
Member

huonw commented Aug 30, 2013

Is there any particular reason the new traits don't inherit from Iterator (or DoubleEndedIterator, etc)?

@bluss
Copy link
Member Author

bluss commented Aug 31, 2013

thanks for the comments.

I think an explicit non-default fn exact_size is a good idea. But, it would leave another place where the iterator declares its size. There is RandomAccessIterator::indexable() too already, but it is not useful for this problem, because it may be defined even for infinite (or larger than uint) iterators.

@huonw
Copy link
Member

huonw commented Sep 1, 2013

It would be neat if we could have something like:

trait ExactSizeHint: Iterator {
    #[test]
    fn test_exact_size_hint() {
        let it = make_a_self(); // the hard part
        assert!(match it.size_hint() {
            (n, Some(m)) => n == m,
            _ => false
        })
    }
}

so the test would be automatically included for every implementation of the trait.... but, it's currently not possible to implement make_a_self.

blake2-ppc added 3 commits September 1, 2013 18:17
Address discussion with acrichto; inherit DoubleEndedIterator so that
`.rposition()` can be a default method, and that the nische of the trait
is clear. Use assertions when using `.size_hint()` in reverse enumerate
and `.rposition()`
bors added a commit that referenced this pull request Sep 3, 2013
The message of the first commit explains (edited for changed trait name):

The trait `ExactSize` is introduced to solve a few small niggles:

* We can't reverse (`.invert()`) an enumeration iterator
* for a vector, we have `v.iter().position(f)` but `v.rposition(f)`.
* We can't reverse `Zip` even if both iterators are from vectors

`ExactSize` is an empty trait that is intended to indicate that an
iterator, for example `VecIterator`, knows its exact finite size and
reports it correctly using `.size_hint()`. Only adaptors that preserve
this at all times, can expose this trait further. (Where here we say
finite for fitting in uint).

---

It may seem complicated just to solve these small "niggles",
(It's really the reversible enumerate case that's the most interesting)
but only a few core iterators need to implement this trait.

While we gain more capabilities generically for some iterators,
it becomes a tad more complicated to figure out if a type has
the right trait impls for it.
@bors bors closed this Sep 3, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 4, 2022
…tiple, r=Manishearth

Fix `manual_range_contains` false negative with chains of `&&` and `||`

Fixes rust-lang#8745

Since the precedence for `&&` is the same as itself the HIR for a chain of `&&` ends up with a right skewed tree like:

```
     &&
    /  \
  &&   c2
 /  \
... c1
```

So only the leftmost `&&` was actually "fully" checked, the top level was just `c2` and `&&` so the `manual_range_contains` lint won't apply. This change makes it also check `c2` with `c1`.

There's a bit of a hacky solution in the [second commit](rust-lang/rust-clippy@257f097) to check if the number of open/closing parens in the snippet match. This is to prevent a case like `((x % 2 == 0) || (x < 0)) || (x >= 10)` from offering a suggestion like `((x % 2 == 0) || !(0..10).contains(&x)` which now won't compile.

Any suggestions for that paren hack welcome, kinda new to working on this so not too sure about possible solutions :) it's weird because I don't know how else to check for parens in HIR considering they're removed when lowering AST.

changelog: Fix [`manual_range_contains`] false negative with chains of `&&` and `||`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants