Skip to content
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

cargo test --all-targets fails #768

Closed
warren2k opened this issue Sep 30, 2023 · 4 comments · Fixed by #770
Closed

cargo test --all-targets fails #768

warren2k opened this issue Sep 30, 2023 · 4 comments · Fixed by #770

Comments

@warren2k
Copy link
Contributor

At least, it fails with toolchain version 1.72.1

@Philippe-Cholet
Copy link
Member

Philippe-Cholet commented Sep 30, 2023

On rust 1.72.0 (stable), thread 'main' panicked at 'attempt to add with overflow', benches\bench1.rs:584:19

fn kmerge_tenway(c: &mut Criterion) {
    let mut data = vec![0; 10240];
    let mut state = 1729u16;
    fn rng(state: &mut u16) -> u16 {
        let new = state.wrapping_mul(31421) + 6927; // <----
        *state = new;
        new
    }
    for elt in &mut data {
        *elt = rng(&mut state);
    }
    // ...

That seems to be quite old code. Investigating further...

@warren2k
Copy link
Contributor Author

Looking further into this, it seems there's no attempt to run this in CI. Maybe this is not an issue.

@Philippe-Cholet
Copy link
Member

Philippe-Cholet commented Sep 30, 2023

It was added in #101 (feb. 2016... ...): https://github.com/rust-itertools/itertools/pull/101/files#diff-5e88c455e47e41d2006104fe8795309466144a7dc0b25f98632e7b85ec90b866R674
I ran cargo test --bench bench1 on old commits (like a year ago and more) and got the same error.
That's definitely weird, good catch anyway.

@Philippe-Cholet
Copy link
Member

Philippe-Cholet commented Sep 30, 2023

Running the associated bench cargo bench "kmerge tenway" is fine though. I don't understand.

...Oh... bench uses release profile so it's silently ignored: wrapping_add would fix this I guess.
EDIT: It does.

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 a pull request may close this issue.

2 participants