-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
Trans refactor part 1 #7124
Conversation
I accidentally deleted the branch. |
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.
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? |
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.
@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. |
Fix lintcheck on windows changelog: None
This removes all of the explicit
@mut
fields fromCrateContext
. 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:
CrateContext
was borrowed in amatch
causing it to be borrowed for the entire length of the match, even though that wasn't wanted (or needed).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.