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 a "by reference" adaptor for AllocRef #71442

Merged
merged 1 commit into from
Apr 23, 2020

Conversation

TimDiekmann
Copy link
Member

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 22, 2020
@Amanieu
Copy link
Member

Amanieu commented Apr 22, 2020

I've been thinking about how we're going to integrate AllocRef and GlobalAlloc and come to the conclusion that we may want to have all AllocRef methods take &self instead of &mut self.

This would allow us to have a blanket AllocRef impl for all types that implement GlobalAlloc, after which we could change #[global_allocator] to require AllocRef instead of GlobalAlloc.

I don't think it will cause any issues in practice since all currently existing allocators use &self in their methods, including bumpalo.

@TimDiekmann
Copy link
Member Author

While it would be possible to simply implement AllocRef for A: GlobalAlloc this should not be the only reason to change this. Quoting the initial RFC:

The justification for &mut self is this:

  • It does not restrict allocator implementors from making sharable allocators: to do so, just do impl<'a> Allocator for &'a MySharedAlloc, as illustrated in the DumbBumpPool example.
  • &mut self is better than &self for simple allocators that are not sharable. &mut self ensures that the allocation methods have exclusive access to the underlying allocator state, without resorting to a lock. (Another way of looking at it: It moves the onus of using a lock outward, to the allocator clients.)

I think this should be discussed in deepth in the WG's repository. Even if we decide to change the signature to &self, then by_ref would be changed to return &Self instead of &mut Self.

@Amanieu
Copy link
Member

Amanieu commented Apr 23, 2020

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 23, 2020

📌 Commit e602680 has been approved by Amanieu

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 23, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 23, 2020
Rollup of 7 pull requests

Successful merges:

 - rust-lang#70633 (Confusing suggestion on incorrect closing `}`)
 - rust-lang#71404 (Don't fuse Chain in its second iterator)
 - rust-lang#71408 (Check code blocks tags)
 - rust-lang#71442 (Add a "by reference" adaptor for `AllocRef`)
 - rust-lang#71446 (Only use read_unaligned in transmute_copy if necessary)
 - rust-lang#71470 (Fix doc links)
 - rust-lang#71479 (add back Scalar::null_ptr)

Failed merges:

r? @ghost
@bors bors merged commit 1363a4b into rust-lang:master Apr 23, 2020
@TimDiekmann TimDiekmann deleted the allocref-mut-ref branch April 24, 2020 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement AllocRef for references to A: AllocRef
4 participants