Closed kinto-b closed 2 months ago
Attention: Patch coverage is 97.22222%
with 1 lines
in your changes are missing coverage. Please review.
Project coverage is 94.55%. Comparing base (
6814180
) to head (568021d
). Report is 64 commits behind head on master.
Files | Patch % | Lines |
---|---|---|
src/flatten_ok.rs | 97.22% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@Philippe-Cholet I was just looking back over the code wondering that myself! Investigating now...
Edit: Have to step out for dinner. Will look into this some more later on. I feel as though it shouldn't work. The necessary fix seems pretty simple, but I want to make the tests fail with the current code before I change anything
Ah so the tests fail to capture what's wrong with this code because they use Option<u8>
values for each Ok item in the iterator. We need non-singleton values.
There is a comment there about it being too slow to use Vec<u8>
:
I've tested changing over to Vec<u8>
and, though it catches the problem, it takes several minutes for the test to finish once the bug is ironed out.
What's the best approach here do you think?
Ah I think I wrote this comment. Use Vec<u8>
here is really slow, it's not really an option. So if we can't give [u8; 3]
instead of Option<u8>
then maybe we can write our own limited vector (what a pain) and implement the necessary traits for this to work. I'm gonna investigate this.
So we basically need a light small vector, here is one with max 2 elements. The flatten_ok
test pass under 1 second for me.
use quickcheck::Arbitrary;
use rand::Rng;
...
// remove comment
fn flatten_ok(v: Vec<Result<SmallIter2<u8>, char>>) -> () {
...
/// Like `VecIntoIter<T>` with maximum 2 elements.
#[derive(Debug, Clone, Default)]
enum SmallIter2<T> {
#[default]
Zero,
One(T),
Two(T, T),
}
impl<T: Arbitrary> Arbitrary for SmallIter2<T> {
fn arbitrary<G: quickcheck::Gen>(g: &mut G) -> Self {
match g.gen_range(0u8, 3) {
0 => Self::Zero,
1 => Self::One(T::arbitrary(g)),
2 => Self::Two(T::arbitrary(g), T::arbitrary(g)),
_ => unreachable!(),
}
}
// maybe implement shrink too, maybe not
}
impl<T> Iterator for SmallIter2<T> {
type Item = T;
fn next(&mut self) -> Option<Self::Item> {
match std::mem::take(self) {
Self::Zero => None,
Self::One(val) => Some(val),
Self::Two(val, second) => {
*self = Self::One(second);
Some(val)
}
}
}
fn size_hint(&self) -> (usize, Option<usize>) {
let len = match self {
Self::Zero => 0,
Self::One(_) => 1,
Self::Two(_, _) => 2,
};
(len, Some(len))
}
}
impl<T> DoubleEndedIterator for SmallIter2<T> {
fn next_back(&mut self) -> Option<Self::Item> {
match std::mem::take(self) {
Self::Zero => None,
Self::One(val) => Some(val),
Self::Two(first, val) => {
*self = Self::One(first);
Some(val)
}
}
}
}
nit: that looks wrong since it never shrinks from EDIT: No, I'm blind and missed the One
to Zero
.take
, nevermind.
(ArrayVec<T, 2>
is the classic solution for an iterator like this, but I would also understand not wanting to add a dev-dep just for this.)
I thought of ArrayVec<T, N>
but I would have to wrap it anyway to implement quickcheck::Arbitrary
(and therefore Iterator
s too). I would have added it if we wanted more control than a fixed max-length of 2.
@Philippe-Cholet Ah neat, thanks for that! Shall I pop that into its own module within tests/
or just stick it in tests/specializations.rs
?
@kinto-b Just add this in "tests/specializations.rs" where it's needed. We can move it later if we need to.
Sidenote: For CI to pass again (because of recent Clippy 1.78), you will need to rebase your branch on master.
Thanks!
Relates to #755
Edit:
.rfold
is an almost identical implementation, so I'll put it through in a moment. Just running benchmarks...Edit: