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

Move the AST from @T to Gc<T>, improve Gc<T> #14250

Merged
merged 3 commits into from
Jun 11, 2014
Merged

Conversation

alexcrichton
Copy link
Member

This commit removes @T from the compiler by moving the AST to using Gc<T>. This also starts treating Gc<T> as @T in the same way that Box<T> is the same as ~T in the compiler.

After this hits a snapshot, the @T syntax should be able to be removed completely.

@alexcrichton
Copy link
Member Author

cc #14193

@alexcrichton
Copy link
Member Author

Note that this also purges all remnants of @T from libcore.

@steveklabnik
Copy link
Member

After this hits a snapshot, the @t syntax should be able to be removed completely.

Wooo!

@pcwalton
Copy link
Contributor

@alexcrichton Some tests failed on Travis.

@alexcrichton
Copy link
Member Author

Oops, forgot to push some changes, tests should be fixed now

@huonw
Copy link
Member

huonw commented May 16, 2014

Reviving #13316 might be nice, although it's likely rebase-hell, given how large it is.

@eddyb
Copy link
Member

eddyb commented May 17, 2014

Uh oh, this happened without mentioning me. It's sad how #13316 went stale without getting into any real decision making process.
I'll attempt cherry-picking each commit on a different branch, rebasing would fail miserably.
@alexcrichton I really hope this didn't take too much time for you (it's easier to automate than P was), as I doubt it will remain in place (or hope that it won't), if it gets merged.

@sfackler
Copy link
Member

Travis is still sad

@alexcrichton
Copy link
Member Author

@eddyb, this didn't take too much time, but I meant for this to be as small of a migration as possible. I didn't want to change the actual representation of the AST, I just wanted to remove all @ pointers from it, and then the language.

I would expect this to make further migrations a little easier because all the autoderef should have been taken care of (in order for this to bootstrap). Sadly, though, it still relies on the implicit copyability of Gc<T>.

@alexcrichton
Copy link
Member Author

Thanks @sfackler, should be addressed now

@@ -73,8 +54,54 @@ impl<T> Clone for Gc<T> {
#[cfg(not(test))]
pub static GC: () = ();

#[cfg(test)]
pub static GC: () = ();
impl<T: Eq + 'static> Eq for Gc<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are buggy because they don't handle cycles, and Gc<T> permits those in valid code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow. This is not a localized problem for Gc<T>, it also affects Rc<T>. Are you proposing we remove the trait implementations for both pointers, or just Gc<T>?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With Rc<T>, it's a bug to create a strong reference cycle as it will leak. It's permitted in correct code for Gc<T> though so it seems bad if ==, etc. can stack overflow. I guess it could shove some metadata into a map in TLS to cope with it (not quite sure).

@thestinger
Copy link
Contributor

I'm okay with this as quick hack to remove @T, as long as the implementations of traits like Eq and Hash will be removed after migrating to Rc<T> in place of this. It would be painful to add all the clone calls, so an incremental step does make sense. The Rust compiler still has memory consumption as the number one issue so tracing garbage collection isn't ever going to make sense.

@huonw
Copy link
Member

huonw commented May 18, 2014

Thanks to @eddyb's hard work, #13316 has been rebased and seems to pass travis.

@alexcrichton
Copy link
Member Author

This PR is meant as a step to removing the @ syntax, it is only fallout that it affected so much of the compiler. I'm ok with waiting for #13316 to reach a decision, but I would like to remove @ sooner rather than later.

This commit uses the same trick as ~/Box to map Gc<T> to @t internally inside
the compiler. This moves a number of implementations of traits to the `gc`
module in the standard library.

This removes functions such as `Gc::new`, `Gc::borrow`, and `Gc::ptr_eq` in
favor of the more modern equivalents, `box(GC)`, `Deref`, and pointer equality.

The Gc pointer itself should be much more useful now, and subsequent commits
will move the compiler away from @t towards Gc<T>

[breaking-change]
bors added a commit that referenced this pull request Jun 11, 2014
This commit removes `@T` from the compiler by moving the AST to using `Gc<T>`. This also starts treating `Gc<T>` as `@T` in the same way that `Box<T>` is the same as `~T` in the compiler.

After this hits a snapshot, the `@T` syntax should be able to be removed completely.
@bors bors merged commit 54c2a1e into rust-lang:master Jun 11, 2014
@alexcrichton alexcrichton deleted the gc branch June 11, 2014 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants