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

Reimplement the most important structs and traits for testing #19

Closed
wants to merge 1 commit into from
Closed

Conversation

TimDiekmann
Copy link
Member

In order to test features, it makes sense to me to reimplement the important parts from rust-lang/rust. After changes to the API were decided in the discussions/issues, the code base should reflect those changes and everyone is aware of the current state.

Currently, this includes Alloc, AllocErr, CannotRealocInPlace, Excess, Layout, LayoutErr, Global and Box.
Additionally, the following functions are adjusted to work with our Layout: alloc, alloc_zeroed, dealloc, realloc, and handle_alloc_error.

However, a few things had to be adjusted to match the alloc crate as soon as possible:

  • the Drop trait of Box calls box_free directly.
  • for Fn, FnOnce, FnMut, and FnBox F has to implement Copy

I did not set up an CI, this is up to the repository owner 🙂

@glandium
Copy link

glandium commented May 5, 2019

There's an independent, but stable-only, version of this in https://crates.io/crates/allocator_api

@Ericson2314
Copy link

Yeah I think it's better to keep the code separate from the wg repo.

@SimonSapin
Copy link
Contributor

@Ericson2314 Why?

@TimDiekmann
Copy link
Member Author

There's an independent, but stable-only, version of this in https://crates.io/crates/allocator_api

@glandium I'm aware of your crate, but as you mentioned it's for the stable toolchain and liballoc is a crate which is using #![feature(...)] a lot. As we want to merge our changes to liballoc, we have to work around nightly features and later revert them. Additionally, I think you don't want to drop semver compatibility, and if only one of the filed issues will be used, it would break it.

Yeah I think it's better to keep the code separate from the wg repo.

@Ericson2314 I don't think we should split up code and discussions. With the code in this repo, it's possible to make PRs, discuss them, and merge the change if we want to keep those.

Currently, this includes `Alloc`, `AllocErr`, `CannotRealocInPlace`, `Excess`, `Layout`, `LayoutErr`, `Global` and `Box`.
Additionally, the following functions are adjusted to work with our `Layout`: `alloc`, `alloc_zeroed`, `dealloc`, `realloc`, and `handle_alloc_error`. 

However, a few things had to be adjusted to match the `alloc` crate as soon as possible: 
- the `Drop` trait of `Box` calls `box_free` directly.
- for `Fn`, `FnOnce`, `FnMut`, and `FnBox` `F` has to implement `Copy`
@TimDiekmann
Copy link
Member Author

Another thing for discussion:

  • should clippy lints be applied? (use_self, default_trait_access, items_after_statement)
  • should impl PartialOrd for Box be reordered to match the trait (lint by IntelliJ)?

@gnzlbg
Copy link

gnzlbg commented May 5, 2019

If something has enough consensus to be merged here (and approval of the libs team), why wouldn't we want to perform the change in rust-lang/rust instead ?

@TimDiekmann
Copy link
Member Author

If something has enough consensus to be merged here (and approval of the libs team), why wouldn't we want to perform the change in rust-lang/rust instead ?

Playing around with liballoc directly takes a lot of time to compile. Even if stage0 is not compiled, it's way faster to maintain a small crate with the basic functionality.

@gnzlbg
Copy link

gnzlbg commented May 9, 2019

So I don't know if this would be very useful. At best, this allows us to say: "hey, this change seems ok, for this thing that's not liballoc", which given how thorny allocators are, doesn't really need to translate to the real world in any meaningful way.

I would be on board with properly forking rust-lang/rust/src/liballoc for the purpose of experimentation somewhere, with its tests, benchmarks, and ideally, also implementations of the Alloc/GlobalAlloc traits for the most widely used allocators (heap, dlmalloc, jemalloc, wee_alloc, etc.), and their tests and benchmarks, as well as some integration tests (e.g., some servo benchmarks).

That would allow us to not only see how the changes proposed as PR impact not only some parts of liballoc, but all of liballoc, its users, run the tests, run the benchmarks, etc. to be able to have some hard data about their impact.

Whether we do this in this repo, or in a different one (e.g. rust-lang/wg-allocators-liballoc), I don't think matters much.

@Ericson2314
Copy link

Ericson2314 commented May 10, 2019

@SimonSapin so we can merge upstream into the code and make PRs from the code. All the things @gnzlbg said.

@TimDiekmann
Copy link
Member Author

@SimonSapin so we can merge upstream into the code and make PRs from the code.

Why this is not possible with this repository? For now we only use this repository for the issue tracker.

@Ericson2314
Copy link

If you merge this into master here as opposed to a separate branch, then when you go to merge back into the main repo it will pull in this readme and what-not?

@gnzlbg
Copy link

gnzlbg commented May 10, 2019 via email

@TimDiekmann TimDiekmann closed this Oct 1, 2019
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.

5 participants