Skip to content
This repository was archived by the owner on Nov 27, 2020. It is now read-only.

Type annotations required everywhere #5

Closed
95th opened this issue Nov 9, 2019 · 12 comments
Closed

Type annotations required everywhere #5

95th opened this issue Nov 9, 2019 · 12 comments

Comments

@95th
Copy link
Contributor

95th commented Nov 9, 2019

The allocator generic Vec constructors ask for type annotations everywhere.

fn foo() {
    let b = Bump::new();
    let v = alloc_wg::vec::Vec::new_in(&b);
    v.push("String");
}

and I get:

error[E0283]: type annotations required: cannot resolve `_: alloc_wg::alloc::BuildAllocRef`
   --> src/lib.rs:186:13
    |
186 |     let v = alloc_wg::vec::Vec::new_in(&b);
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: required by `alloc_wg::vec::Vec::<T, B>::new_in`

This applies to all constructors of Vec type (and String type I am working on).

I can get around that by specifying:

let v: Vec<_, &Bump> = alloc_wg::vec::Vec::new_in(&b);

But it's annoying to do it everywhere. Is it possible to have inferred?

@95th
Copy link
Contributor Author

95th commented Nov 9, 2019

You can try it out using https://github.com/95th/bumpalo

@95th
Copy link
Contributor Author

95th commented Nov 9, 2019

I think in the below code (and other places), we should accept B instead of B::Ref since we're getting that in the end anyway.

    pub fn new_in(mut a: B::Ref) -> Self {
        let capacity = if mem::size_of::<T>() == 0 { !0 } else { 0 };
        Self {
            ptr: NonNull::dangling(),
            capacity,
            build_alloc: a.get_build_alloc(),
            _owned: PhantomData,
        }
    }

@95th
Copy link
Contributor Author

95th commented Nov 9, 2019

But get_build_alloc is in DeallocRef and children. :(

@95th
Copy link
Contributor Author

95th commented Nov 9, 2019

More reduced example:

This doesn't work:

use alloc_wg::vec::Vec;
use alloc_wg::alloc::{Global, AbortAlloc};

fn main() {
    let mut v = Vec::new_in(AbortAlloc(Global));
    v.push("String");
}

This works:

use alloc_wg::vec::Vec;
use alloc_wg::alloc::{Global, AbortAlloc};

fn main() {
    let mut v: Vec<_, AbortAlloc<Global>> = Vec::new_in(AbortAlloc(Global));
    v.push("String");
}

But weirdly, this one works too:

use alloc_wg::vec::Vec;
use alloc_wg::alloc::{Global, AbortAlloc};

fn main() {
    let mut v: Vec<_,> = Vec::new_in(AbortAlloc(Global));
    v.push("String");
}

Probably some bug in rust. It's not. It's just same as Vec<_>

@TimDiekmann
Copy link
Owner

TimDiekmann commented Nov 9, 2019 via email

@TimDiekmann
Copy link
Owner

TimDiekmann commented Nov 10, 2019

I'm currently thinking of another new and with_capacity method: fn new_with(build_alloc: B) -> Vec<T, B> and fn with_capacity_with(capacity: usize, build_alloc: B -> Vec<T, B>.

advantages:

  • Inferred allocator
  • You don't have to create an AllocRef yourself

downsides:

  • Two new methods (or even more if we have try_with_capacity and/or (try_)with_capacity_zeroed)

It would also be possible to change B::Ref to B in constructors, you could still access the builder with another method. What do you think?

@95th
Copy link
Contributor Author

95th commented Nov 11, 2019

Inference is good but then we have two kinds of methods. Is it possible to provide getter for Self::Ref in BuildAlloc rather than get_build_alloc in DeallocRef?

@95th
Copy link
Contributor Author

95th commented Nov 11, 2019

Something like:

pub trait BuildAllocRef: Sized + Clone {
    type Ref: DeallocRef<BuildAlloc = Self>;

    fn get_alloc(&mut self) -> Self::Ref;
}

pub trait DeallocRef: Sized {
    type BuildAlloc: BuildAllocRef<Ref = Self>;

    unsafe fn dealloc(&mut self, ptr: NonNull<u8>, layout: NonZeroLayout);
}

@95th
Copy link
Contributor Author

95th commented Nov 11, 2019

Please take a look at https://github.com/95th/alloc-wg

@95th
Copy link
Contributor Author

95th commented Nov 11, 2019

Then this works:

fn main() {
    let a = AbortAlloc(Global);
    let mut v = Vec::new_in(a);
    v.push(100);
}

@TimDiekmann
Copy link
Owner

I think this is the best solution possible 🙂

@95th
Copy link
Contributor Author

95th commented Nov 12, 2019

Awesome. Thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants