Skip to content

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

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

Closed
nikomatsakis opened this issue Feb 28, 2014 · 17 comments · Fixed by #12654
Closed

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

nikomatsakis opened this issue Feb 28, 2014 · 17 comments · Fixed by #12654
Labels
E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. P-low Low priority

Comments

@nikomatsakis
Copy link
Contributor

To the extent possible, we should try to have all interior mutability be done via Cell, RefCell, Atomic, or (not yet landed) Volatile. If one of these patterns doesn't suffice, it'd be nice to know why!

@nikomatsakis
Copy link
Contributor Author

Nominating for P-Low

@nikomatsakis
Copy link
Contributor Author

Tagging as E-Mentor; I can mentor.

@nikomatsakis
Copy link
Contributor Author

Related to #12577

Other types that use interior mutability I can think of off the top of my head:

  • Arena
  • RwArc

@pongad
Copy link
Contributor

pongad commented Mar 1, 2014

@nikomatsakis I'm definitely interested. Though---having never been a part of a project like this before---not quite sure how the mentoring works just yet :P

@edwardw
Copy link
Contributor

edwardw commented Mar 2, 2014

This is the Rc part: rewrite Rc using Cell.

@thestinger
Copy link
Contributor

I don't understand the reasoning behind doing this for Rc. Using a marker type makes the intent clearer, and will perform better with optimizations disabled. The Rc type is also supposed to be Freeze when T is Freeze and this will make it always non-Freeze. The Arc type should also be Freeze.

@nikomatsakis
Copy link
Contributor Author

@thestinger Marker types do not seem clearer to me than Cell, quite the opposite, but of course thi is a matter of opinion.

At first I thought you were on to something regarding Freeze, but then I realize that I was mistaken. In particular, we need to settle on precisely what makes something not Freeze: must the interior mutability be something interior, something that you own, or just something you can reach?

But this question is orthogonal to whether we use markers or Cell. If I understand it, the use of Cell would look something like this:

struct RcBox<T> {
    value: T,
    strong: Cell<uint>,
    weak: Cell<uint>
}

pub struct Rc<T> {
    priv ptr: *mut RcBox<T>,
}

In this case, whether or not Rc is Freeze will hinge directly on what we decide about owning vs mutability. At present, the definition is based on reachability. But this definition may be stronger than what we need. I guess it depends on just what Freeze is to be used for.

So after considering this some more, I still think that using Cell is the right call for Rc is the right call, but the real discussion is what Freeze precisely ought to mean, which is most properly discussed I think under #11781.

@thestinger make sense?

@thestinger
Copy link
Contributor

I don't think we have a way of enforcing deep immutability. There are many existing Freeze types doing internal writes due to I/O. I don't know of a purpose for Freeze beyond the high-level semantic meaning (perceived inherited mutability) and type-based alias analysis (which is not deep) so that's the lens I'm viewing it through.

@thestinger
Copy link
Contributor

Every type using memory allocation needs to become non-Freeze if means deep immutability. The free function is mutating memory via the pointer. So it's not just a problem with I/O.

@nikomatsakis
Copy link
Contributor Author

@thestinger I don't believe that is an accurate summray of what was proposed, but it is a moot point, as in the latest plan there is no Freeze.

@pongad
Copy link
Contributor

pongad commented Mar 9, 2014

Is there a change to Atomic coming? Arc is already using AtomicUint.

@nikomatsakis
Copy link
Contributor Author

I think @edwardw and I decided that Arc is already using the right primitives and it was just Rc that wasn't.

@nikomatsakis
Copy link
Contributor Author

Though I didn't dig too deeply, maybe @alexcrichton can confirm. But it looked to me like it was using the proper abstraction (is there more than one?)

@alexcrichton
Copy link
Member

Indeed, Arc is correctly implemented as-is it appears.

@pongad
Copy link
Contributor

pongad commented Mar 9, 2014

@nikomatsakis I'm running into tests not compiling because something doesn't implement Freeze. Should I wait for #12577 to land before I continue?

@pongad
Copy link
Contributor

pongad commented Mar 9, 2014

I actually did not realized that @edwardw is already on this issue. I'll let you handle this one :D

@pnkfelix
Copy link
Member

Assigning P-low as suggested.

bors added a commit that referenced this issue Mar 21, 2014
Since `Arc` has been using `Atomic`, this closes 12625.

Closes #12625.
@bors bors closed this as completed in db5206c Mar 21, 2014
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 25, 2022
…eykril

fix: completes non exhaustive variant within the defining crate

close rust-lang#12624
flip1995 pushed a commit to flip1995/rust that referenced this issue Apr 18, 2024
…cation, r=blyxyas

fix incorrect suggestion for `!(a as type >= b)`

fixes rust-lang#12625

The expression  `!(a as type >= b)` got simplified to `a as type < b`, but because of rust's parsing rules that `<` is interpreted as a start of generic arguments for `type`.  This is fixed by recognizing this case and adding extra parens around the left-hand side of the comparison.

changelog: [`nonminimal_bool`]: fix incorrect suggestion for  `!(a as type >= b)`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. P-low Low priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants