Rewrite rc::Rc using cell::Cell #12654

edwardw · 2014-03-02T20:23:07Z

Since Arc has been using Atomic, this closes 12625.

alexcrichton · 2014-03-02T20:35:30Z

Using a RefCell adds an extra word to all Rc values, perhaps the strong and weak counts should be Cell<uint> instead?

edwardw · 2014-03-02T20:39:56Z

Good point. I'll try Cell then.

thestinger · 2014-03-02T22:02:10Z

Can this wait until the issue is discussed? I don't think Rc should become non-Freeze, and I think the marker type is much clearer anyway.

alexcrichton · 2014-03-02T22:19:42Z

It seems wrong to say that Rc is Freeze because whenever it's cloned through an &-pointer it's modified. If we were to inform LLVM that the pointer is entirely immutable or use this in a threaded context, seems like things could go wrong.

To me, this is the reason why markers should not be used in favor of types with safe interfaces like Cell.

thestinger · 2014-03-02T22:39:59Z

No, nothing would go wrong if we told LLVM an & pointer to Rc is immutable. There's no concept of deep immutability in LLVM. The only relevant pointer is the *mut one used for the box itself, which is already correct. You can't use it in a threaded context because it won't be Share or Send.

If the meaning of Freeze is changing, then Rc/Arc are far from the only types that need to become non-Freeze. Performing I/O is technically doing writes to memory... so deep immutability really means that all the methods have to be pure.

nikomatsakis · 2014-03-03T13:52:11Z

I agree we definitely want Cell<T> and not RefCell, if we make this change.

I also agree with @thestinger, we ought to discuss. Whether this is the right call seems to hinge on just what we want the definition of Freeze to be. I was going under the assumption that Rc<T> ought not to be freeze, but maybe I was mistaken. The question is whether reaching mutable data or owning mutable data is the def'n of Freeze.

I finally think this PR is not the place to have such a discussion.

nikomatsakis · 2014-03-03T13:58:44Z

After some more thought I have modified my opinion and I am still in favor of using Cell. See the issue for details. (But this is not to say we shouldn't discuss a bit further.)

edwardw · 2014-03-03T15:44:42Z

The patch has been updated to use Cell. And some tests in syntax::ast are failing precisely because Rc doesn't inherit Freeze from T any more.

nikomatsakis · 2014-03-04T13:06:03Z

The patch looks good! We probably do want to tweak the def'n of Freeze from "reaches mutable" to "interior mutability". Presumably we should do that before we land this patch.

alexcrichton · 2014-03-07T02:03:22Z

The current plan is to possibly remove the Freeze trait entirely (to be superseded by the Share trait). The main impetus for this change is that interior mutability is not done through *mut T, but rather through Cell<T>. For that reason, the counts should use Cell<uint> rather than uint directly through the *mut pointer.

I don't have much of a preference on whether this uses *mut RcBox<T> or *RcBox<T>, although I would favor *RcBox<T> because it more accurately reflects that Rc<T> is not deeply mutable. @nikomatsakis may have some clarifications to this reasoning.

Implementation-wise, it seems odd to introduce a one-off trait for these two pointer types. Sure it's not exported, but there's no other clean way to do it? I feel like something returning &RcBox<T> is the best way about this.

nikomatsakis · 2014-03-07T14:26:23Z

I agree that *RcBox<uint> is correct.

As far as the trait goes, it seemed to me like an impl detail that wouldn't be reflected in the docs and just allowed us to shave off some duplicate code, which seems like a good thing. @alexcrichton you disagree?

thestinger · 2014-03-07T16:04:28Z

I don't really find * to be the right choice here. The free function takes *mut, and it's quite pointless to make the distinction if we're going to ignore it.

nikomatsakis · 2014-03-07T16:15:44Z

@thestinger I see. Yes, I guess you're right, if we think of *T as *const T in C (as we ought to) then *mut is indeed correct.

nikomatsakis · 2014-03-07T16:16:04Z

/me not a big fan of our current treatment of *

alexcrichton · 2014-03-07T17:31:21Z

Doesn't that seem a little odd? By that same logic, nothing should be *T, everything should be *mut T, which seems backwards?

thestinger · 2014-03-07T18:36:15Z

I'm not sure what logic you're referencing. The free function takes *mut because it uses the pointer to mutate memory, and the pointer is no longer valid after the call. If that's not a case where we're going to use *mut, then there's no reason to have it. I do think the distinction is useful, just as const is useful in C, because it does catch lots of mistakes.

The const-correctness rules in C++ are that you can coerce a non-const pointer/reference to const, but you can't go back the other way. In this case, it needs to remain *mut after the call to malloc because it is going to eventually be passed to free. It can be coerced to * to pass it to a function that's not going to mutate it.

alexcrichton · 2014-03-09T08:07:46Z

@edwardw, this patch needs a rebase, and could you also make these changes?

*RcBox<T> => *mut RcBox<T>
Change the RcBoxPtr trait to have fn inner(&'a self) -> &'a RcBox<T>;?

edwardw · 2014-03-09T14:42:53Z

How about fn inner<'a>(&'a self) -> &'a mut RcBox<T>; ? Since it is already mut, I'm thinking there's not much need to use Cell any more.

edwardw · 2014-03-09T16:17:12Z

The latest commit demonstrates the fn inner<'a>(&'a self) -> &'a mut RcBox<T>; idea. It is more of refactoring than rewriting now. Still kind nice, but I find the patch ... a bit thin.

alexcrichton · 2014-03-09T18:13:17Z

The point of this patch is to not use &mut RcBox<T> sadly. Having an &mut pointer is a guarantee that you are the only thread in the world looking at this pointer, but that is not true in the case of Rc (due to clones). For this reason, the correct signature is returning &'a RcBox<T> and using Cell for interior mutability.

nikomatsakis · 2014-03-09T18:19:24Z

(What @alexcrichton said)

edwardw · 2014-03-09T18:53:49Z

So Cell it is. Note that some unit tests in syntax are failing because of Rc now doesn't inherit Freeze from T. So I guess this patch shouldn't be merged yet.

alexcrichton · 2014-03-09T19:52:21Z

I'd rather not leave this lingering, is it possible to fix the tests before the kind inference is updated?

edwardw · 2014-03-09T19:59:20Z

Sure. Maybe with an FIXME in it?

alexcrichton · 2014-03-09T20:02:05Z

Is it really necessary to have a FIXME? Can the test be modified?

edwardw · 2014-03-09T20:10:29Z

Unfortunately, they specifically test for being frozen: ast.rs and interner.rs.

alexcrichton · 2014-03-09T20:22:42Z

The test in ast.rs is fine to ignore, and I don't understand why the Freeze bound is on the interner. Why are the tests in interner.rs failing but compilation succeeding?

edwardw · 2014-03-09T20:25:53Z

I guess because no other place tries to instantiate Interner with something has Rc in it such as RcStr in the test.

alexcrichton · 2014-03-09T20:26:51Z

Can you switch the tests over to just using ~str or a similar type?

edwardw · 2014-03-09T20:28:08Z

Certainly. I'll try to submit a new patch then.

alexcrichton · 2014-03-20T16:34:24Z

Closing due to inactivity, but feel free to reopen with a rebase!

edwardw · 2014-03-21T07:19:22Z

Rebased. Since Unsafe has landed, Freeze shouldn't be a concern any more. Reopen the ticket, @alexcrichton ?

edwardw · 2014-03-21T08:24:01Z

r?

flaper87 · 2014-03-21T09:23:51Z

src/libstd/rc.rs

    }
 }

+#[allow(missing_doc)]


Could you please add some docs? I'd like us to avoid merging patches that add undocumented features.

This is an internal implementation trait, I think, so I'm not sure that docs are needed since these methods aren't publicly available and their purpose is fairly clear.

Fair enough.

Side Note: I personally prefer to have everything documented even internal and pretty obvious things. I think it's good for the language and to make newcomers life easier.

Since `Arc` has been using `Atomic`, this closes 12625. Closes rust-lang#12625.

Since `Arc` has been using `Atomic`, this closes 12625. Closes #12625.

…ust-lang#14367) There were two bugs here. Let's assume `T` is a singleton type implementing `Default` and that `f()` takes a `T`: - `f(<T>::default())` cannot be replaced by `f(<T)` as it was (incorrect spans – this is tricky because the type relative path uses a base span covering only `T`, not `<T>`) (third commit) - The argument of `f(<_>::default())` is inferred correctly, but cannot be replaced by `<_>` or `_`, as this cannot be used to infer an instance of a singleton type (first commit). The second commit offers better error messages by pointing at the whole expression. Fix rust-lang#12654 changelog: [`default_constructed_unit_struct`]: do not suggest incorrect fix when using a type surrounded by brackets

edwardw mentioned this pull request Mar 2, 2014

Rewrite Rc and Arc to use Cell and Atomic respectively #12625

Closed

alexcrichton closed this Mar 20, 2014

alexcrichton reopened this Mar 21, 2014

flaper87 reviewed Mar 21, 2014
View reviewed changes

Rewrite rc::Rc using cell::Cell

db5206c

Since `Arc` has been using `Atomic`, this closes 12625. Closes rust-lang#12625.

bors added a commit that referenced this pull request Mar 21, 2014

auto merge of #12654 : edwardw/rust/rc-arc, r=huonw

a0f943c

Since `Arc` has been using `Atomic`, this closes 12625. Closes #12625.

bors merged commit db5206c into rust-lang:master Mar 21, 2014

edwardw deleted the rc-arc branch March 21, 2014 15:18

Rewrite rc::Rc using cell::Cell #12654

Rewrite rc::Rc using cell::Cell #12654

Conversation

edwardw commented Mar 2, 2014

alexcrichton commented Mar 2, 2014

edwardw commented Mar 2, 2014

thestinger commented Mar 2, 2014

alexcrichton commented Mar 2, 2014

thestinger commented Mar 2, 2014

nikomatsakis commented Mar 3, 2014

nikomatsakis commented Mar 3, 2014

edwardw commented Mar 3, 2014

nikomatsakis commented Mar 4, 2014

alexcrichton commented Mar 7, 2014

nikomatsakis commented Mar 7, 2014

thestinger commented Mar 7, 2014

nikomatsakis commented Mar 7, 2014

nikomatsakis commented Mar 7, 2014

alexcrichton commented Mar 7, 2014

thestinger commented Mar 7, 2014

alexcrichton commented Mar 9, 2014

edwardw commented Mar 9, 2014

edwardw commented Mar 9, 2014

alexcrichton commented Mar 9, 2014

nikomatsakis commented Mar 9, 2014

edwardw commented Mar 9, 2014

alexcrichton commented Mar 9, 2014

edwardw commented Mar 9, 2014

alexcrichton commented Mar 9, 2014

edwardw commented Mar 9, 2014

alexcrichton commented Mar 9, 2014

edwardw commented Mar 9, 2014

alexcrichton commented Mar 9, 2014

edwardw commented Mar 9, 2014

alexcrichton commented Mar 20, 2014

edwardw commented Mar 21, 2014

edwardw commented Mar 21, 2014

flaper87 Mar 21, 2014

Choose a reason for hiding this comment

nikomatsakis Mar 21, 2014

Choose a reason for hiding this comment

flaper87 Mar 21, 2014

Choose a reason for hiding this comment