Skip to content

Trans refactor part 1 #7124

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
wants to merge 5 commits into from
Closed

Conversation

Aatch
Copy link
Contributor

@Aatch Aatch commented Jun 14, 2013

This removes all of the explicit @mut fields from CrateContext. There are still a few that are managed, but no longer do we have @mut bool in the structure.

Most of the changes are changing @CrateContext to @mut CrateContext, though I did change as many as I could get away with to &CrateContext and &mut CrateContext. The biggest thing preventing me from changing to &[mut] in most places was the instruction counter thing. In two cases, where I got a static borrow error and a dynamic borrow error, I opted to remove the count call there as it was literally the only thing preventing me from switching to &mut CrateContext parameters in both cases.

Other things to note:

  • the EncoderContext uses borrowed pointers with lifetimes, since it can, though that required me to work around the limitation of not being able to move a structure with borrowed pointers into a heap closure. I changed as much as I could to stack closures, but unfortunately I hit the AST visitor and changing that is somewhat outside the scope of this PR. Instead (and there is a comment to this effect) I choose to unsafely get the structure into the heap, this is because I know the lifetimes involved are safe, even though the compiler can't prove it.
  • Many of the changes are workarounds because of the borrow checker, either dynamically freezing it for too long, or inferring too large a scope. This is mostly just from nested function calls where each borrow is considered to last for the entire statement. Other cases are where CrateContext was borrowed in a match causing it to be borrowed for the entire length of the match, even though that wasn't wanted (or needed).
  • I haven't yet tested to see if this changes compilation times in any way. I doubt there will be much of an impact however, as the only major improvements are less indirection and fewer refcount bumps.
  • This lays the foundations to remove many more heap allocations in trans as many cases can be changed to use lifetimes instead.

This change includes some other, minor refactorings, as I am planning a series, however I don't want to submit them all at once as it will be hell to continually rebase.

@Aatch Aatch closed this Jun 14, 2013
@Aatch
Copy link
Contributor Author

Aatch commented Jun 15, 2013

I accidentally deleted the branch.

@Aatch Aatch reopened this Jun 15, 2013
@Aatch Aatch mentioned this pull request Jun 15, 2013
Aatch added 5 commits June 16, 2013 09:17
Move construction of a CrateContext into a static method on
CrateContext.
Remove all the explicit @mut-fields from CrateContext, though many
fields are still @-ptrs.
This required changing every single function call that explicitly
took a @CrateContext, so I took advantage and changed as many as I
could get away with to &-ptrs or &mut ptrs.
@Aatch
Copy link
Contributor Author

Aatch commented Jun 15, 2013

@graydon @brson @pcwalton Someone please look at this, every other PR that gets merged conflicts with this one, I'd like to have this merged before I'm ready with part 2.

@graydon
Copy link
Contributor

graydon commented Jun 15, 2013

I'm on a phone today, detailed review outside scope of what I can do. I would not object to you placing r=me on it, if it does in fact do what the comments say, and if you're willing to do followups on anything I or others pick out in later review?

bors added a commit that referenced this pull request Jun 15, 2013
This removes all of the explicit `@mut` fields from `CrateContext`. There are still a few that are managed, but no longer do we have `@mut bool` in the structure.

Most of the changes are changing `@CrateContext` to `@mut CrateContext`, though I did change as many as I could get away with to `&CrateContext` and `&mut CrateContext`. The biggest thing preventing me from changing to `&[mut]` in most places was the instruction counter thing. In two cases, where I got a static borrow error and a dynamic borrow error, I opted to remove the count call there as it was literally the only thing preventing me from switching to `&mut CrateContext` parameters in both cases.

Other things to note:

* the EncoderContext uses borrowed pointers with lifetimes, since it can, though that required me to work around the limitation of not being able to move a structure with borrowed pointers into a heap closure. I changed as much as I could to stack closures, but unfortunately I hit the AST visitor and changing that is somewhat outside the scope of this PR. Instead (and there is a comment to this effect) I choose to unsafely get the structure into the heap, this is because I know the lifetimes involved are safe, even though the compiler can't prove it.

* Many of the changes are workarounds because of the borrow checker, either dynamically freezing it for too long, or inferring too large a scope. This is mostly just from nested function calls where each borrow is considered to last for the entire statement. Other cases are where `CrateContext` was borrowed in a `match` causing it to be borrowed for the entire length of the match, even though that wasn't wanted (or needed).

* I haven't yet tested to see if this changes compilation times in any way. I doubt there will be much of an impact however, as the only major improvements are less indirection and fewer refcount bumps.

* This lays the foundations to remove many more heap allocations in trans as many cases can be changed to use lifetimes instead.

=====

This change includes some other, minor refactorings, as I am planning a series, however I don't want to submit them all at once as it will be hell to continually rebase.
@Aatch
Copy link
Contributor Author

Aatch commented Jun 15, 2013

@graydon that's fine. I included all the major strokes in the comments, and it seems to work locally. As I said, I am planning on a series of changes to try and modernize trans as best I can so I'm more than willing to follow up any comments.

@bors bors closed this Jun 15, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request Apr 27, 2021
Fix lintcheck on windows

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.

3 participants