AllocRef support for BTreeMap #21
Conversation
|
Thanks for the PR, I'll take a look into this as soon as possible (probably next week). |
| .eq(data.clone().into_iter().filter(|x| *x < key)) | ||
| ); | ||
| assert!(right.into_iter().eq(data.into_iter().filter(|x| *x >= key))); | ||
| } |
There was a problem hiding this comment.
We might want at least some test with an allocator.
There was a problem hiding this comment.
I'm not sure how to approach this, since BTreeMap::new() does create a BTreeMap with an allocator, it just happens to be the global one, all the tests already use an allocator. I think there would be value in having tests with an allocator that's not the global one, in case there's some behavior that can differ in other allocators.
exrook
force-pushed
the
master
branch
2 times, most recently
from
d79b3ea
to
abddef1
Compare
|
You might want to allow clippy errors on the file, I don't think there is a point in changeing all those errors in this crate, this should be changed upstream. |
|
Alright, I've reverted those changes and disabled the warnings. The clippy (and maybe rustfmt) check seems to be failing because it doesn't support running on a fork. |
|
Could you provide a diff-link for your changes and the upstream implementation? Otherwise it's almost impossible to review 🙂 |
|
exrook/alloc-wg@168bac8...master Here's the functional changes made. exrook/alloc-wg@67fd9ee...168bac8 Here are the changes I needed to make to get the code to compile outside of stdlib. |
|
Changes looking good, however there are still around 200 clippy errors (stopped counting at 150). Also, rustfmt still has one error. Running clippy on a fork runs fine, it just don't create inline annotations. |
exrook
force-pushed
the
master
branch
3 times, most recently
from
8770be6
to
53d68cf
Compare
|
Thank you for the effort! |
Hello, I have modified
BTreeMapto optionally work withAllocRefinstead of the global allocator.The diff shown in the PR is extremely large because I first had to copy BTreeMap into the repo, you can view the commit with just the changes for AllocRef support here: 40196f9
The code passes the stdlib
BTreeMapunit tests but I have not yet tested the new apis supporting AllocRef, hopefully the changes I've made so far can be further validated and serve as a starting point for more detailed design discussion.