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

Add CollectIn, FromIteratorIn traits to allow for easier construction #125

Merged
merged 7 commits into from
Oct 18, 2021

Conversation

dreamsmasher
Copy link
Contributor

@dreamsmasher dreamsmasher commented Oct 15, 2021

Currently, building Bump-parameterized collections from iterators is a little verbose, e.g.

use Bumpalo{Bump, collections::Vec};
let bump = Bump::new();
let xs =  Vec::from_iter_in((0..10), &bump);

while a normal Vec can be constructed directly using .collect(). Having to wrap an entire chain in a method call is ugly for longer expressions, and I find myself using .apply(|iter| Vec::from_iter_in(iter, &bump)) from the apply crate a lot to avoid extra indentation.

This PR adds 2 new traits, FromIteratorIn, and an extension trait CollectIn on all Iterators that allows for simpler constructor of bump-parameterized structures. Currently, Vec and String implement FromIteratorIn, and future collections should try to do the same if possible.

On iterators, CollectIn allows a more chainable syntax for constructing these collections:

let bump = Bump::new();
let xs = (0..10).collect_in::<Vec<i32>>(&bump);

Quickcheck-based tests are also included and passing.

Copy link
Owner

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good to me, but needs a cargo fmt before merging.

}


}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make sure there is a newline at the end of the file? I think running cargo fmt will do this.

@fitzgen
Copy link
Owner

fitzgen commented Oct 18, 2021

(Prior art, for posterity: #12)

@dreamsmasher
Copy link
Contributor Author

(Prior art, for posterity: #12)

Oh wow, didn't see this prior implementation (mainly because I grepped for CollectIn instead of collect_in?). Would you like things to be moved to a collections_traits module still?

@fitzgen
Copy link
Owner

fitzgen commented Oct 18, 2021

Would you like things to be moved to a collections_traits module still?

No, this PR is fine, since it isn't modifying the files that are borrowed from std. Thanks!

@fitzgen
Copy link
Owner

fitzgen commented Oct 18, 2021

https://github.com/fitzgen/bumpalo/pull/125/checks?check_run_id=3929433788#step:6:80

error[E0277]: can't compare `[i32]` with `std::vec::Vec<i32>`
  --> tests/collect_in.rs:25:14
   |
25 |     bump_ref == &input
   |              ^^ no implementation for `[i32] == std::vec::Vec<i32>`
   |
   = help: the trait `std::cmp::PartialEq<std::vec::Vec<i32>>` is not implemented for `[i32]`
   = note: required because of the requirements on the impl of `std::cmp::PartialEq<&std::vec::Vec<i32>>` for `&[i32]`

@dreamsmasher
Copy link
Contributor Author

https://github.com/fitzgen/bumpalo/pull/125/checks?check_run_id=3929433788#step:6:80

error[E0277]: can't compare `[i32]` with `std::vec::Vec<i32>`
  --> tests/collect_in.rs:25:14
   |
25 |     bump_ref == &input
   |              ^^ no implementation for `[i32] == std::vec::Vec<i32>`
   |
   = help: the trait `std::cmp::PartialEq<std::vec::Vec<i32>>` is not implemented for `[i32]`
   = note: required because of the requirements on the impl of `std::cmp::PartialEq<&std::vec::Vec<i32>>` for `&[i32]`

Gotta love deref coercion changes. Should be fixed now (tested with cargo +1.44.0 test --features collections,boxed)

Comment on lines 1899 to 1908
impl<'bump, T> FromIteratorIn<T> for Vec<'bump, T> {
type Alloc = &'bump Bump;
fn from_iter_in<I>(iter: I, alloc: Self::Alloc) -> Self
where
I: IntoIterator<Item = T>,
{
Vec::from_iter_in(iter, alloc)
}
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you avoid modifying the files that are copied from std? This makes it easier to update these files in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't realize these were straight from std. I'll move the impls back to their original place, sorry!

Copy link
Owner

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@fitzgen fitzgen merged commit fdc19d5 into fitzgen:master Oct 18, 2021
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.

2 participants