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

Revert "De-share ast::Ty" #7644

Closed
wants to merge 1 commit into from
Closed

Conversation

Aatch
Copy link
Contributor

@Aatch Aatch commented Jul 8, 2013

Revert the removal of the @ from Ty as I think it caused the memory regression seen here in #7615. The graph posted on that PR is from before this change.

Since Ty is a recursive type, I think that the change from @ to ~ meant that a copy that would previously stop at the @ continued. It seems to me that during syntax folding, the data was being kept around somehow, causing the bloat.

This reverts commit 47eca21.

@thestinger
Copy link
Contributor

@bors: retry

@graydon
Copy link
Contributor

graydon commented Jul 18, 2013

From @cmr on the mailing list: "@Aatch said it is broken on Windows with no obvious fix besides de-sharing the rest of the AST"

How so? Why would this be platform specific?

@Aatch
Copy link
Contributor Author

Aatch commented Jul 19, 2013

@graydon, I think this is some mis-communication.

The error that keeps arising is platform specific, only appearing on the windows bot. I don't get any similar error here and have no idea what might cause it.

The fix of completely removing @ from the AST is what I described as the "proper" way to fix it. It's very hard however, while not much code relies on the sharing of @ some does, and it's enough to make completely removing @ very difficult.

@graydon
Copy link
Contributor

graydon commented Jul 19, 2013

Sure, I'm just curious why there's anything platform specific in here at all.

@graydon
Copy link
Contributor

graydon commented Jul 27, 2013

continued in #8072

@graydon graydon closed this Jul 27, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request Sep 8, 2021
Rustup

r? `@ghost`

changelog: none
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.

5 participants