Skip to content

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

Merged
merged 3 commits into from
Nov 9, 2019

Conversation

hamishknight
Copy link
Contributor

Remove the remaining external uses of TypeChecker's Context field, and make it private along with Diags.

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.

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.
@hamishknight hamishknight requested a review from CodaFi November 8, 2019 19:07
@hamishknight
Copy link
Contributor Author

@swift-ci please smoke test

@@ -556,6 +553,9 @@ class TypeChecker final {
std::vector<AbstractClosureExpr *> ClosuresWithUncomputedCaptures;

private:
ASTContext &Context;
DiagnosticEngine &Diags;
Copy link
Contributor

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?

Copy link
Contributor Author

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);
Copy link
Contributor

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.

Copy link
Contributor Author

@hamishknight hamishknight Nov 9, 2019

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!

Copy link
Contributor

@CodaFi CodaFi left a 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.

@CodaFi
Copy link
Contributor

CodaFi commented Nov 9, 2019

⛵️

@CodaFi CodaFi merged commit 4bf9025 into swiftlang:master Nov 9, 2019
@hamishknight hamishknight deleted the decontextualize branch November 9, 2019 23:05
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.

2 participants