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

VecDeque::push_back(()) does not work correctly. #28488

Closed
dpc opened this issue Sep 18, 2015 · 15 comments
Closed

VecDeque::push_back(()) does not work correctly. #28488

dpc opened this issue Sep 18, 2015 · 15 comments
Labels
regression-from-stable-to-stable Performance or correctness regression from one stable version to another.

Comments

@dpc
Copy link
Contributor

dpc commented Sep 18, 2015

http://is.gd/NY4tCl

use std::collections::VecDeque;

fn main() {
    let mut v = VecDeque::new();
    v.push_back(());                                                                                                         
    assert!(v.len() > 0 );   
}

thread '<main>' panicked at 'assertion failed: v.len() > 0', <anon>:6
@agbaroni
Copy link
Contributor

() is void type / value, so it should be correct; if you add nothinh to an empty collection, the collection remains empty ...

@utkarshkukreti
Copy link
Contributor

VecDeque::push_front() too:

use std::collections::VecDeque;

fn main() {
    let mut v = VecDeque::new();
    v.push_front(());                                                                                                         
    assert_eq!(v.len(), 1);   
}

fails with

thread '<main>' panicked at 'assertion failed: `(left == right)` (left: `2`, right: `1`)', <anon>:6

@jonas-schievink
Copy link
Contributor

@ituxbag No, the collection just contains one nothing ;)

() is just a zero-sized type, adding it to collections stores 0 bytes, but 1 element. For example, it works fine with aVec<()>:

fn main() {
    let mut v = Vec::new();
    v.push(());
    v.push(());
    assert_eq!(v.len(), 2);
}

@steveklabnik
Copy link
Member

/cc @rust-lang/libs this is interesting.

@apasel422
Copy link
Contributor

This variant appears to loop forever, presumably in drop since it outputs 2 (!) first:

use std::collections::VecDeque;

fn main() {
    let mut v = VecDeque::new();
    v.push_front(());
    println!("{}", v.len());
}

@bluss bluss added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Sep 18, 2015
@bluss
Copy link
Member

bluss commented Sep 18, 2015

Let's aim to improve the testsuite with general collections + Zero sized types tests too.

@bluss
Copy link
Member

bluss commented Sep 18, 2015

Ok, Zero sized types (ZSTs) are a can of worms, potentially there may be several problems, but so far I found that the VecDeque in Rust 1.3 with ZST uses capacity = !0 which is not a power of two. Now how to fix this..

@Gankra
Copy link
Contributor

Gankra commented Sep 18, 2015

I would recommend isize::MAX + 1, giving a capacity of isize::MAX elements -- I'm becoming more swayed that this is a reasonable upper bound to select unconditionally.

@Gankra
Copy link
Contributor

Gankra commented Sep 18, 2015

Oh, right this capacity is inherited from RawVec! Annoying, but not a big deal.

@bluss
Copy link
Member

bluss commented Sep 18, 2015

This used to work in Rust 1.0, 1.1, 1.2. Regression report, ad-hoc via travis

First broken release: Rust 1.3.0.

@brson brson added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Sep 18, 2015
bors added a commit that referenced this issue Sep 19, 2015
VecDeque: Use power of two capacity even for zero sized types

VecDeque depends on using a power of two capacity. Use the largest
possible power of two capacity for ZSTs.

Fixes #28488
@bluss
Copy link
Member

bluss commented Sep 19, 2015

@dpc As soon as a new nightly is built, it would be great if you could verify that the bug was fixed for the original problem.

@dpc
Copy link
Contributor Author

dpc commented Sep 19, 2015

Sure. I'll try that tomorrow.

@bluss
Copy link
Member

bluss commented Sep 23, 2015

@dpc Any luck?

@dpc
Copy link
Contributor Author

dpc commented Sep 23, 2015

The original problem reported confirmed it's fixed in: https://github.com/dpc/mioco/issues/55#issuecomment-142004340

@bluss
Copy link
Member

bluss commented Sep 23, 2015

Thank you! Good to have some confidence so that we can backport the fix to beta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-stable Performance or correctness regression from one stable version to another.
Projects
None yet
Development

No branches or pull requests

9 participants