Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upSplit the common usage patterns off the `Alloc` trait #50436
Conversation
rust-highfive
assigned
cramertj
May 3, 2018
This comment has been minimized.
This comment has been minimized.
|
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
label
May 3, 2018
This comment has been minimized.
This comment has been minimized.
|
Assigning to someone on libs-- r? @sfackler |
rust-highfive
assigned
sfackler
and unassigned
cramertj
May 3, 2018
glandium
force-pushed the
glandium:split_alloc
branch
from
6c3c160
to
157817c
May 4, 2018
This comment has been minimized.
This comment has been minimized.
|
FWIW this looks like a good step forward to me. IMO if nobody is using these we should just kill them in the future. Those who want to re-add them should make a point for them. |
This comment has been minimized.
This comment has been minimized.
|
As discussed on irc, I think once we have |
glandium
referenced this pull request
May 4, 2018
Closed
Remove bounds on alloc_one, dealloc_one, alloc_array, realloc_array, … #50414
This comment has been minimized.
This comment has been minimized.
|
Added #50414 on top. |
This comment has been minimized.
This comment has been minimized.
|
@alexcrichton what do you think here? This is not the pattern used elsewhere in the standard library for convenience default impls (e.g on Another approach could be adding these as methods as impls on impl Alloc {
pub fn alloc_one<T>(&mut self) -> Result<Unique<T>, AllocErr> { ... }
} |
This comment has been minimized.
This comment has been minimized.
|
Oh, I guess the direct impl approach probably wouldn't work in a non-trait object context. |
This comment has been minimized.
This comment has been minimized.
|
I'd personally prefer to stick to the pattern we have elsewhere in the standard library which is to leave them all in the original trait. It's true that if they're overridden then wrappers won't guarantee they're called and definitions can't assume they're called either. Despite that though this remains the most ergonomic way to call them (only import one trait instead of two) and it leaves the possibility to implement speedier wrappers, even if it's hard to adhere to |
This comment has been minimized.
This comment has been minimized.
|
A downside is that users potentially need to import two traits instead of just one. |
This comment has been minimized.
This comment has been minimized.
|
What's the advantage of EDIT: the only utility I see is that |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
That's indeed very useful. |
This comment has been minimized.
This comment has been minimized.
|
I think that with respect to
Having I like the option of requiring that Therefore I am more in favor of deprecation with removal once |
This comment has been minimized.
This comment has been minimized.
|
Ping from triage @sfackler! What's the status of this PR? |
This comment has been minimized.
This comment has been minimized.
|
I think we want to document the semantics of how these methods interact and leave them in Alloc. |
pietroalbini
assigned
sfackler
and unassigned
sfackler
May 21, 2018
pietroalbini
added
S-waiting-on-author
and removed
S-waiting-on-review
labels
May 21, 2018
This comment has been minimized.
This comment has been minimized.
|
Ping from triage @glandium! The reviewer asked for some changes, can you do them? |
This comment has been minimized.
This comment has been minimized.
|
Per #50436 (comment) and #50436 (comment) , let's just close this. |
glandium commentedMay 3, 2018
The
Alloctrait keeps functions likealloc,dealloc,realloc,etc. and a new
AllocExttrait providesalloc_one,dealloc_one,alloc_array, etc. While there are some hypothetical benefits frombeing able to have custom implementation for the latter, it's far from
the most common need, and when you need to implement generic wrappers
(which I've found to be incredibly common, to the point I've actually
started to write a custom derive for that), you still need to implement
all of them because the wrappee might be customizing them.
OTOH, if an
Alloctrait implementer does try to do fancy things ine.g.
alloc_one, they're not guaranteed thatdealloc_onewill becalled for that allocation. There are multiple reasons for this:
The helpers are not used consistently. Just one example,
raw_vecuses a mix of
alloc_array,alloc/alloc_zeroed, but only usesdealloc.Even with consistent use of e.g.
alloc_array/dealloc_array, onecan still safely convert a
Vecinto aBox, which would then usedealloc.Then there are some parts of the API that just don't exist (no
zeroed version of
alloc_one/alloc_array)So even though there are actual use cases for specialization of e.g.
alloc_one(and as a matter of fact, I do have such a need formozjemalloc), one is better off using a specialized allocator instead.
Actually, it's worse than that, in the rust repo, there's exactly one
use of
alloc_array, and no use ofalloc_one,dealloc_one,realloc_array,dealloc_array. Not even box syntax usesalloc_one,it uses
exchange_malloc, which takes asizeandalign. So thosefunctions are more meant as a convenience for clients than for
implementers.
The
AllocExttrait is implemented for all types implementingAllocautomatically, allowing clients to opt into using its methods, while
preventing implementers from shooting themselves in the foot by trying
to do fancy things by overriding those methods (and making it easier on
people implementing proxy allocators).