User-defined placement-box #18233

pnkfelix · 2014-10-22T15:17:43Z

Add support for user-defined placement-box expressions (i.e. box (<place>) <value> for types other than Box<T>).

This is a prototype design that @nikomatsakis and @pnkfelix have been working on. It works by desugaring an instance of placement-box into a protocol where we first create the intermediate storage with a pseudo-out-pointer, then initialize the storage, and then convert the out-pointer into the desired box type.

The code was originally written to handle both box (<place>) <value> and box <value> via the same uniform desugaring, and it is easy to revise support for the latter; but as is, this PR only puts box (<place>) <value> into the desugaring. The reason for this is that the error messages get a lot nastier to handle from the desugared form, and pnkfelix did not want to hold this work up further trying to resolve that.

The final design for this will require an RFC; this code is entirely experimental for now.

Proto RFC: http://discuss.rust-lang.org/t/pre-rfc-placement-box-with-placer-trait/729/5

Note that this narrows desugaring to only be when actual placer is provided; `box <value_expr>` and `box () <value_expr>` both still fall through to the `mk_unary(UnUniq, _)` path. Many more compile-fail tests would need updating if we were not restricting desugaring to `box` with explicit placer.

rust-highfive · 2014-10-22T15:17:48Z

Warning

These commits modify unsafe code. Please review it carefully!

pnkfelix · 2014-10-22T15:17:54Z

r? @nikomatsakis

pnkfelix · 2014-10-22T15:33:36Z

agh failed make tidy ... will fix pronto.

(okay, fixed now; earlier review request still stands. :)

pnkfelix · 2014-10-22T15:59:14Z

(on reflection, I'm not sure why I thought printing from drop would be any better than failing from drop. I will revise those to just call fail as well.)

thestinger · 2014-10-22T16:10:13Z

src/liballoc/boxed.rs

+    ExchangeHeapSingleton { _force_singleton: () };
+
+/// This the singleton type used solely for `boxed::HEAP`.
+pub struct ExchangeHeapSingleton { _force_singleton: () }


The term exchange heap really shouldn't be used. It's not how dynamic allocation works in Rust. It's wrong to refer to this as HEAP at all. The Box<T> type represents a dynamic allocation from the same allocator as other types like Vec<T> and Rc<T>. Rust doesn't draw any distinction between memory on a "heap" and other memory mappings.

thestinger · 2014-10-22T16:14:42Z

The box placement protocol should be designed with the same flexibility as placement new / perfect forwarding in C++. It should be possible to push onto a vector by building the element in-place rather than constructing it and copying it inside. I don't think these concerns are being addressed by the design effort put into box, so I'm against progressing with it further.

thestinger · 2014-10-22T16:16:17Z

There needs to be a clear plan on how this is going to work for use cases like vector or hash table inserts before I'm comfortable with it, because I don't want to see Rust end up with an inadequate language feature that needs to be duplicated.

…d on Vec.

nikomatsakis · 2014-10-23T18:26:49Z

@thestinger Good point, that is an important use case. I don't see any trouble expressing something like emplace_back. (In fact, @pnkfelix just pushed a commit showing how it can be done: b293984). What do you think? In any case, as @pnkfelix said, we plan to write an RFC, so it'd be great to get a good list of any other requirements you have in mind so that we can write up how the design handles them.

thestinger · 2014-10-23T18:50:25Z

If it can support emplace_back then I think it's flexible enough to cover most use cases of perfect forwarding / placement new in C++. I can't think of another major use case for it at the moment.

This made some things cleaner (e.g. I got to get rid of the `Cell` in the `AtomPool` example). But it also highlights some weaknesses in my current expansion -- namely, my use of top-level functions rather than methods means that I am currently forced to make sure I have the right level of borrowing in the PLACER input in `box (PLACER) EXPR`. I will look into my options there -- it may be that the right thing is just to switch to method invocations.

nikomatsakis · 2014-11-06T15:42:29Z

src/liballoc/boxed.rs

+/// the fact that the `align_of` intrinsic currently requires the
+/// input type to be Sized (which I do not think is strictly
+/// necessary).
+pub struct IntermediateBox<Sized? T>{


Since this is public, we'll be committing to the continued existence of this type, at least. I'm ok with this but cc @aturon. We'll probably want to bikeshed the name :)

(Of course there's no way to avoid having this be public, at least not without private trait methods at the very least...)

In particular I expect we want to reuse the "agent" term

On Thu, Nov 06, 2014 at 07:48:41AM -0800, Felix S Klock II wrote:

(Of course there's no way to avoid having this be public, at least not without private trait methods at the very least...)

Yes. And I think that's ok.

nikomatsakis · 2014-11-06T15:55:03Z

src/libsyntax/ext/expand.rs

+            // implement the Placer trait.  Likewise, the actual
+            // expansion below actually calls free functions in
+            // `std::ops::placer`, in order to avoid the need for UFCS
+            // (and also to hopefully produce better error messags).


I think just using Placer::place(&mut <place_expr>) and so on will have a similar effect

nikomatsakis · 2014-11-06T16:23:30Z

It probably makes sense to leave these for follow-up PRs, but it'd be nice to have placer implementations for:

Rc
Arc
&TypedArena (as discussed)
emplace_back, perhaps something with hashtable insert, etc

We should perhaps make a tracker issue (after this lands) for those sorts of things. Not a huge deal as they can be added as we like.

pnkfelix · 2014-11-06T16:39:12Z

(added link to Proto RFC on discuss to the description; you can see it here: http://discuss.rust-lang.org/t/pre-rfc-placement-box-with-placer-trait/729/5 )

steveklabnik · 2014-12-02T08:35:39Z

This needs a rebase.

pnkfelix · 2014-12-16T12:37:36Z

(closing; I no longer think this is on the 1.0 timeframe)

pnkfelix · 2015-03-11T09:31:34Z

see #22086

pnkfelix added 2 commits October 22, 2014 14:09

Add tests for user-defined use of box (<place>) <value>.

53865bc

placate make tidy.

040cf98

thestinger reviewed Oct 22, 2014
View reviewed changes

pnkfelix added 3 commits October 23, 2014 18:07

Revise the protocol to avoid inadvertantly moving the Placer.

452ecab

A new test: illustrate use of box with emplacing into back of Vec.

731e9ba

Simplify example so that emplace_back is "just" an additional metho…

b293984

…d on Vec.

pnkfelix mentioned this pull request Oct 26, 2014

How box should work rust-lang/rfcs#413

Open

nikomatsakis mentioned this pull request Oct 27, 2014

placement box #11779

Closed

nikomatsakis self-assigned this Oct 30, 2014

nikomatsakis reviewed Nov 6, 2014
View reviewed changes

pnkfelix mentioned this pull request Nov 18, 2014

Require public trait items to be marked pub rust-lang/rfcs#227

Closed

pnkfelix closed this Dec 16, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User-defined placement-box #18233

User-defined placement-box #18233

pnkfelix commented Oct 22, 2014

rust-highfive commented Oct 22, 2014

pnkfelix commented Oct 22, 2014

pnkfelix commented Oct 22, 2014

pnkfelix commented Oct 22, 2014

thestinger Oct 22, 2014

thestinger commented Oct 22, 2014

thestinger commented Oct 22, 2014

nikomatsakis commented Oct 23, 2014

thestinger commented Oct 23, 2014

nikomatsakis Nov 6, 2014

pnkfelix Nov 6, 2014

nikomatsakis Nov 6, 2014

nikomatsakis Nov 6, 2014

nikomatsakis Nov 6, 2014

nikomatsakis commented Nov 6, 2014

pnkfelix commented Nov 6, 2014

steveklabnik commented Dec 2, 2014

pnkfelix commented Dec 16, 2014

pnkfelix commented Mar 11, 2015

User-defined placement-box #18233

User-defined placement-box #18233

Conversation

pnkfelix commented Oct 22, 2014

rust-highfive commented Oct 22, 2014

pnkfelix commented Oct 22, 2014

pnkfelix commented Oct 22, 2014

pnkfelix commented Oct 22, 2014

thestinger Oct 22, 2014

Choose a reason for hiding this comment

thestinger commented Oct 22, 2014

thestinger commented Oct 22, 2014

nikomatsakis commented Oct 23, 2014

thestinger commented Oct 23, 2014

nikomatsakis Nov 6, 2014

Choose a reason for hiding this comment

pnkfelix Nov 6, 2014

Choose a reason for hiding this comment

nikomatsakis Nov 6, 2014

Choose a reason for hiding this comment

nikomatsakis Nov 6, 2014

Choose a reason for hiding this comment

nikomatsakis Nov 6, 2014

Choose a reason for hiding this comment

nikomatsakis commented Nov 6, 2014

pnkfelix commented Nov 6, 2014

steveklabnik commented Dec 2, 2014

pnkfelix commented Dec 16, 2014

pnkfelix commented Mar 11, 2015