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

Update Alloc trait to nightly, add nightly feature #68

Closed
wants to merge 8 commits into from

Conversation

hansihe
Copy link

@hansihe hansihe commented Feb 29, 2020

@hansihe
Copy link
Author

hansihe commented Mar 5, 2020

I meant to get back to this earlier when I saw that the build failed.

It looks like --default-toolchain $RUSTUP_TOOLCHAIN might not be respected in https://github.com/fitzgen/bumpalo/blob/master/ci/install-rust.yml#L4? Even though nightly is passed to rust_version, stable is installed.

I'll poke around a bit

@hansihe
Copy link
Author

hansihe commented Mar 5, 2020

Okey, so it seems like the alloc trait is under major flux right this moment.

I guess it would make sense to wait until things have settled a bit to update and merge?

I presume what you want to do is to track the (feature flagged) alloc trait present in the stable releases?

@hansihe
Copy link
Author

hansihe commented Mar 5, 2020

Yeah, let's hold off on this until most changes to the AllocRef (formerly alloc) trait are out of the way.

rust-lang/hashbrown#133 (comment)

@fitzgen
Copy link
Owner

fitzgen commented Mar 5, 2020

Sounds like a plan!

@NOBLES5E
Copy link

Sounds like a plan!

AllocRef is on nightly now. Let's start working on merging this? 😊

@fitzgen
Copy link
Owner

fitzgen commented Nov 30, 2020

AllocRef is on nightly now. Let's start working on merging this? blush

Sounds good. Unclear to me whether this PR should be revived or whether starting from scratch would be easier. I don't have time to drive it forward, but I can review the eventual implementation.

@NOBLES5E
Copy link

AllocRef is on nightly now. Let's start working on merging this? blush

Sounds good. Unclear to me whether this PR should be revived or whether starting from scratch would be easier. I don't have time to drive it forward, but I can review the eventual implementation.

It seems that this PR only misses the latest two commits (which only contains some modification in the readme file).

@hansihe What do you think? Let's revive this PR?

@hansihe
Copy link
Author

hansihe commented Dec 25, 2020

Both Box and Vec in nightly have an allocator parameter now. hashbrown also has an allocator parameter on both HashMap and HashSet, and they should land on their std counterparts soon too.

For me at least, things in the Rust ecosystem have progressed to the point where I would be fine with bumpalo just providing the Bump arena, along with an AllocRef implementation.

My fork, located here, updates the traits to latest nightly, but also removes all collections from the codebase.

It's understandable if @fitzgen would prefer to keep the collections in the crate for now. If that's the case, I will be maintaining my fork as the effort is pretty minimal. If someone else would like to contribute an updated collections implementation on top of my fork in order to get things merged back here, I would be really happy with that too.

@fitzgen
Copy link
Owner

fitzgen commented Jan 5, 2021

Yes, I'd like to keep the collections module until the custom allocators ship on stable.

In the meantime, I'd still like to have an AllocRef/Allocator/whatever-it-ends-up-being-called implementation behind an unstable cargo flag. If we can lift that from your fork, that would be great.

I would be fine with bumpalo just providing the Bump arena, along with an AllocRef implementation.

Yep, this is the eventual goal but we need custom allocators on stable before we remove the collections module.

(I don't mind you maintaining a fork, or course, but out of curiosity: what would your fork have over the main crate other than removing the collections module? Since the collections module is off by default and requires a cargo flag to enable, it doeTsn't seem like forking is really getting you that much, unless there are additional changes.)

@hansihe
Copy link
Author

hansihe commented Jan 5, 2021

I just don't have the time right now to update the different collections to the newest version of the trait, so I won't be able to make a PR that includes that. If this repo lands an update to the trait, then that's brilliant and I'll of course switch to using that directly instead.

@baloo baloo mentioned this pull request Jan 16, 2021
@fitzgen
Copy link
Owner

fitzgen commented Jan 29, 2021

Fixed in #92.

@fitzgen fitzgen closed this Jan 29, 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.

3 participants