-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Make TypeChecker's Context and Diags fields private #28152
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
Conversation
Flip things so rather than storing a TypeChecker and asking for the ASTContext when needed, store an ASTContext, and ask for a TypeChecker when needed.
@swift-ci please smoke test |
@@ -556,6 +553,9 @@ class TypeChecker final { | |||
std::vector<AbstractClosureExpr *> ClosuresWithUncomputedCaptures; | |||
|
|||
private: | |||
ASTContext &Context; | |||
DiagnosticEngine &Diags; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much work would it be to remove Diags
altogether?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CodaFi Ah nice, there's only one use. I'll remove it future PR :)
@@ -3658,7 +3676,7 @@ bool TypeChecker::isAvailabilitySafeForConformance( | |||
} | |||
|
|||
void TypeChecker::typeCheckDecl(Decl *D) { | |||
DeclChecker(*this).visit(D); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typeCheckDecl
can be static. That should remove some other type checker dependencies in the constraint system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CodaFi Yup, I'm planning on sinking all of the getLegacyGlobalTypeChecker
calls down to the places where we actually access the remaining type checker state. Thanks for the review!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. My comments are directional, not nitpicks.
⛵️ |
Remove the remaining external uses of TypeChecker's
Context
field, and make it private along withDiags
.Also remove TypeChecker fields from a few walkers, flipping things such that instead of storing a TypeChecker and asking for the ASTContext when needed, store an ASTContext, and ask for a TypeChecker when needed.