Skip to content

[NFC] Drop PerformJobsState as a friend class #17864

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

Conversation

CodaFi
Copy link
Contributor

@CodaFi CodaFi commented Jul 10, 2018

Make the coupling between PerformJobsState and Compilation indirect and expose some convenient accessors in the process.

Make the coupling between PerformJobsState and Compilation indirect.
@CodaFi CodaFi requested review from davidungar and jrose-apple July 10, 2018 21:57
@CodaFi
Copy link
Contributor Author

CodaFi commented Jul 10, 2018

@swift-ci please smoke test

Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh. It was written this way originally because PerformJobsState was a sort of "fake extension" to Compilation, but I guess it really wasn't that closely tied after all. This seems reasonable to me.

@CodaFi
Copy link
Contributor Author

CodaFi commented Jul 10, 2018

⛵️

@CodaFi CodaFi merged commit 4023105 into swiftlang:master Jul 10, 2018
@CodaFi CodaFi deleted the friendship-ended-with-PerformJobsState branch July 10, 2018 23:53
@davidungar
Copy link
Contributor

I was surprised that you asked for my review and then merged so quickly before I could get to it. However, it’s a very innocuous change! I like the decoupling.

@CodaFi
Copy link
Contributor Author

CodaFi commented Jul 11, 2018

@davidungar Sorry about that. I usually tag all the stakeholders I can think of as a way to cast a wide net rather than gather a list of gates for smaller patches like these. Thank you for the vote of confidence!

@davidungar
Copy link
Contributor

@CodaFi Thanks for responding. In the future if you put me down as a reviewer, if you want me to assume that you specifically want my review, please let me know somehow. Otherwise I'll assume that, as with this PR, you are casting that wide net.

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