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

Implement placement-in protocol for relevant data structures #30172

Closed
nagisa opened this Issue Dec 2, 2015 · 15 comments

Comments

Projects
None yet
@nagisa
Contributor

nagisa commented Dec 2, 2015

We currently have the unstable new arrow syntax (<-) for placement-in in nightly with intention to see how people like the syntax/how ergonomic it is/etc. However, it is pretty hard to try the syntax out without having any useful/common data-structures implementing the placement-in protocol and thus it is pretty unlikely for us to collect much feedback on the syntax.

The Placer, Place and InPlace traits should be implemented for following data structures as appropriate:

It should also be investigated how viable implementing placement protocol is for other std::collections collections and possibly implementing these as well.

Since both placement protocol and placement-in syntax are unstable, there’s no stability hazard.

cc @pnkfelix.


I personally feel this issue has tons of potential to help people get acquainted with rust’s standard library (i.e. is in E-easy land) and would be willing to answer any questions (E-mentor) regarding implementation strategies on IRC or the issue itself.

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Dec 2, 2015

Member

we might consider myvec.back() <- value; rather than myvec <- value;. the main reason I can think of of hand is that there may be issues related to indexing sugars, e.g. ambiguity of mymap[k] <- value; when you have a map with vec values.

Member

pnkfelix commented Dec 2, 2015

we might consider myvec.back() <- value; rather than myvec <- value;. the main reason I can think of of hand is that there may be issues related to indexing sugars, e.g. ambiguity of mymap[k] <- value; when you have a map with vec values.

@apasel422

This comment has been minimized.

Show comment
Hide comment
@apasel422

apasel422 Dec 4, 2015

Member

@nagisa BTreeMap is also a candidate through its Entry.

Member

apasel422 commented Dec 4, 2015

@nagisa BTreeMap is also a candidate through its Entry.

@nagisa

This comment has been minimized.

Show comment
Hide comment
@nagisa

nagisa Dec 4, 2015

Contributor

@apasel422 added to the list.

Contributor

nagisa commented Dec 4, 2015

@apasel422 added to the list.

@nagisa

This comment has been minimized.

Show comment
Hide comment
@nagisa

nagisa Feb 17, 2016

Contributor

Since the LinkedList has been implemented one could use the PR as example when implementing the protocol for other collections.

Contributor

nagisa commented Feb 17, 2016

Since the LinkedList has been implemented one could use the PR as example when implementing the protocol for other collections.

@notmatt

This comment has been minimized.

Show comment
Hide comment
@notmatt

notmatt Feb 18, 2016

@nagisa this looks like something I'd like to take a swing at; would some initial code in a week (±a weekend) be ok?

notmatt commented Feb 18, 2016

@nagisa this looks like something I'd like to take a swing at; would some initial code in a week (±a weekend) be ok?

@nagisa

This comment has been minimized.

Show comment
Hide comment
@nagisa

nagisa Feb 18, 2016

Contributor

Sure!
On Feb 18, 2016 10:12 PM, "Matthew Smillie" notifications@github.com
wrote:

@nagisa https://github.com/nagisa this looks like something I'd like to
take a swing at; would some initial code in a week (±a weekend) be ok?


Reply to this email directly or view it on GitHub
#30172 (comment).

Contributor

nagisa commented Feb 18, 2016

Sure!
On Feb 18, 2016 10:12 PM, "Matthew Smillie" notifications@github.com
wrote:

@nagisa https://github.com/nagisa this looks like something I'd like to
take a swing at; would some initial code in a week (±a weekend) be ok?


Reply to this email directly or view it on GitHub
#30172 (comment).

@notmatt

This comment has been minimized.

Show comment
Hide comment
@notmatt

notmatt Feb 28, 2016

@nagisa quick update: apart from an unexpected shortage of spare time, this is going well. I've mostly been reading/understanding Placer et alia & the LinkedList implmentation. I'm starting with the VecDeque given its broad similarity, but not a lot to show yet.

notmatt commented Feb 28, 2016

@nagisa quick update: apart from an unexpected shortage of spare time, this is going well. I've mostly been reading/understanding Placer et alia & the LinkedList implmentation. I'm starting with the VecDeque given its broad similarity, but not a lot to show yet.

@pcwalton

This comment has been minimized.

Show comment
Hide comment
@pcwalton

pcwalton Aug 8, 2016

Contributor

Wanted for WebRender.

Contributor

pcwalton commented Aug 8, 2016

Wanted for WebRender.

@nagisa

This comment has been minimized.

Show comment
Hide comment
@nagisa

nagisa Dec 12, 2016

Contributor

Dear T-libs members. It has been a

  • bit more than a year since this issue has been created;
  • 3 quarters since #32366 where there was a note that there has to be some conventions before this can proceed;
  • 1 quarter since Servo expressed their immense interest in this feature;
  • 1 quarter since there seemingly was a discussion by T-libs members with no aftermath posted whatsoever.

Perhaps the boring, cold and silent cooldown period after Christmas could be a good time to ponder on whatever you think is the best and report after the vacation?

I’d like to note that gaining experience with placement-in is literally blocking stabilisation of this feature and since nothing implements the placement-in stuff, nobody is using it, ergo not gaining experience. I feel like adding the placement-in impls, be it in an unstable form, is the easiest way to break this loop that’s putting this awesome feature to the cold hands of Ms. Banshee.

Contributor

nagisa commented Dec 12, 2016

Dear T-libs members. It has been a

  • bit more than a year since this issue has been created;
  • 3 quarters since #32366 where there was a note that there has to be some conventions before this can proceed;
  • 1 quarter since Servo expressed their immense interest in this feature;
  • 1 quarter since there seemingly was a discussion by T-libs members with no aftermath posted whatsoever.

Perhaps the boring, cold and silent cooldown period after Christmas could be a good time to ponder on whatever you think is the best and report after the vacation?

I’d like to note that gaining experience with placement-in is literally blocking stabilisation of this feature and since nothing implements the placement-in stuff, nobody is using it, ergo not gaining experience. I feel like adding the placement-in impls, be it in an unstable form, is the easiest way to break this loop that’s putting this awesome feature to the cold hands of Ms. Banshee.

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Dec 19, 2016

Member

@nagisa Thanks for the reminder! It looks like what happened here is that the libs team discussion was posted to the PR rather than the tracking issue.

To recap:

  • Collections like Vec which have a clear "default place" can implement the protocol directly, e.g. vec <- item.
  • Otherwise standardizing around the convention collection.place_location() <- item seems reasonable. For example vec.place_front() <- item

But then it turned out we can't quite get the desired sweet syntax for Vec yet, so we're proposing using place_location as the convention. PRs along these lines continue to be welcome.

(In terms of priority, placement continues to not be a high priority as far as I'm aware.)

Member

aturon commented Dec 19, 2016

@nagisa Thanks for the reminder! It looks like what happened here is that the libs team discussion was posted to the PR rather than the tracking issue.

To recap:

  • Collections like Vec which have a clear "default place" can implement the protocol directly, e.g. vec <- item.
  • Otherwise standardizing around the convention collection.place_location() <- item seems reasonable. For example vec.place_front() <- item

But then it turned out we can't quite get the desired sweet syntax for Vec yet, so we're proposing using place_location as the convention. PRs along these lines continue to be welcome.

(In terms of priority, placement continues to not be a high priority as far as I'm aware.)

bors added a commit that referenced this issue Jan 7, 2017

Auto merge of #38551 - aidanhs:aphs-vec-in-place, r=brson
Implement placement-in protocol for `Vec`

Follow-up of #32366 per comment at #30172 (comment), updating to latest rust, leaving @apasel422 as author and putting myself as committer.

I've removed the implementation of `push` in terms of place to make this PR more conservative.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Jan 19, 2017

Rollup merge of #39062 - martinhath:placement-in-binaryheap, r=nagisa
Implement placement-in protocol for `BinaryHeap`

Related to #30172, and loosley based on #38551.

At the moment, this PR is in a pretty rough state, but I wanted to get some feedback to see if I'm going in the right direction.

I hope the Mentor label of #30172 is still applicable, even though it's a year old 😄

bors added a commit that referenced this issue Jan 20, 2017

Auto merge of #39062 - martinhath:placement-in-binaryheap, r=nagisa
Implement placement-in protocol for `BinaryHeap`

Related to #30172, and loosley based on #38551.

At the moment, this PR is in a pretty rough state, but I wanted to get some feedback to see if I'm going in the right direction.

I hope the Mentor label of #30172 is still applicable, even though it's a year old 😄

arielb1 added a commit to arielb1/rust that referenced this issue Mar 9, 2017

Rollup merge of #40389 - F001:placementVecDeque, r=nagisa
Implement placement-in protocol for `VecDeque`

CC #30172

r? @nagisa

arielb1 added a commit to arielb1/rust that referenced this issue Mar 10, 2017

Rollup merge of #40389 - F001:placementVecDeque, r=nagisa
Implement placement-in protocol for `VecDeque`

CC #30172

r? @nagisa

arielb1 added a commit to arielb1/rust that referenced this issue Mar 10, 2017

Rollup merge of #40389 - F001:placementVecDeque, r=nagisa
Implement placement-in protocol for `VecDeque`

CC #30172

r? @nagisa

arielb1 added a commit to arielb1/rust that referenced this issue Mar 10, 2017

Rollup merge of #40389 - F001:placementVecDeque, r=nagisa
Implement placement-in protocol for `VecDeque`

CC #30172

r? @nagisa

arielb1 added a commit to arielb1/rust that referenced this issue Mar 10, 2017

Rollup merge of #40389 - F001:placementVecDeque, r=nagisa
Implement placement-in protocol for `VecDeque`

CC #30172

r? @nagisa

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 10, 2017

Rollup merge of #40389 - F001:placementVecDeque, r=nagisa
Implement placement-in protocol for `VecDeque`

CC #30172

r? @nagisa

alexcrichton added a commit to arielb1/rust that referenced this issue Mar 10, 2017

Rollup merge of #40389 - F001:placementVecDeque, r=nagisa
Implement placement-in protocol for `VecDeque`

CC #30172

r? @nagisa

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 10, 2017

Rollup merge of #40389 - F001:placementVecDeque, r=nagisa
Implement placement-in protocol for `VecDeque`

CC #30172

r? @nagisa

arielb1 added a commit to arielb1/rust that referenced this issue Mar 10, 2017

Rollup merge of #40389 - F001:placementVecDeque, r=nagisa
Implement placement-in protocol for `VecDeque`

CC #30172

r? @nagisa

arielb1 added a commit to arielb1/rust that referenced this issue Mar 10, 2017

Rollup merge of #40389 - F001:placementVecDeque, r=nagisa
Implement placement-in protocol for `VecDeque`

CC #30172

r? @nagisa

arielb1 added a commit to arielb1/rust that referenced this issue Mar 10, 2017

Rollup merge of #40389 - F001:placementVecDeque, r=nagisa
Implement placement-in protocol for `VecDeque`

CC #30172

r? @nagisa

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 10, 2017

Rollup merge of #40389 - F001:placementVecDeque, r=nagisa
Implement placement-in protocol for `VecDeque`

CC #30172

r? @nagisa

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 11, 2017

Rollup merge of #40389 - F001:placementVecDeque, r=nagisa
Implement placement-in protocol for `VecDeque`

CC #30172

r? @nagisa

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 11, 2017

Rollup merge of #40389 - F001:placementVecDeque, r=nagisa
Implement placement-in protocol for `VecDeque`

CC #30172

r? @nagisa

frewsxcv added a commit to frewsxcv/rust that referenced this issue Mar 12, 2017

Rollup merge of #40390 - F001:placementHashMap, r=nagisa
Implement placement-in protocol for `HashMap`

CC #30172

r? @nagisa

@steveklabnik steveklabnik removed the A-libs label Mar 24, 2017

@nagisa nagisa referenced this issue Mar 27, 2017

Closed

Tracking issue for placement new #27779

0 of 4 tasks complete
@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Sep 16, 2017

Contributor

API issue: reusing Vec's placer type allows violating memory safety (writing to the Vec without allocating space). See issue #44637

Contributor

bluss commented Sep 16, 2017

API issue: reusing Vec's placer type allows violating memory safety (writing to the Vec without allocating space). See issue #44637

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Sep 16, 2017

Contributor

(Edited: This ended up in the wrong issue, transporting the comment over to the tracking issue for placement new.)

Contributor

bluss commented Sep 16, 2017

(Edited: This ended up in the wrong issue, transporting the comment over to the tracking issue for placement new.)

@F001

This comment has been minimized.

Show comment
Hide comment
@F001

F001 Jan 15, 2018

Contributor

I also wonder whether it is useful to implement placement-in protocol for many wrapper types beside collections, such as Option Cell RefCell etc.

Contributor

F001 commented Jan 15, 2018

I also wonder whether it is useful to implement placement-in protocol for many wrapper types beside collections, such as Option Cell RefCell etc.

@tamird

This comment has been minimized.

Show comment
Hide comment
@tamird

tamird Apr 8, 2018

Contributor

Looks like this is headed in the opposite direction now? rust-lang/rfcs#2387

Contributor

tamird commented Apr 8, 2018

Looks like this is headed in the opposite direction now? rust-lang/rfcs#2387

@estebank

This comment has been minimized.

Show comment
Hide comment
@estebank

estebank Apr 18, 2018

Contributor

Closing as per removal of placement features in #48333.

Contributor

estebank commented Apr 18, 2018

Closing as per removal of placement features in #48333.

@estebank estebank closed this Apr 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment