Skip to content
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

Incremental build refactor #10538

Merged
merged 10 commits into from
Nov 25, 2020
Merged

Incremental build refactor #10538

merged 10 commits into from
Nov 25, 2020

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented Nov 24, 2020

This is the second and hopefully the final attempt at refactoring incremental build to simply use arrays and mutations instead of the framework that was built for it.

The goal here is not to gain performance; but to improve the readability and flexibility without being constrained by the previous framework.

@auduchinok
Copy link
Member

If it's OK to use arrays and mutation now, would it also make sense to incorporate changes from #9259 somehow?

@TIHan
Copy link
Contributor Author

TIHan commented Nov 24, 2020

If it's OK to use arrays and mutation now, would it also make sense to incorporate changes from #9259 somehow?

The nature of incremental builder is mutable state. The changes made in this PR are implementation details and don't bleed out of the API. Though I would want to make fileNames and referencedAssemblies be ImmutableArrays.

@TIHan
Copy link
Contributor Author

TIHan commented Nov 24, 2020

This is now ready. :)

@@ -979,7 +980,7 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
computeStampedFileName cache ctok slot fileInfo (fun slot fileInfo ->
let prevSemanticModel =
match slot with
| 0 -> initial
| 0 (* first file *) -> initial
Copy link
Member

Choose a reason for hiding this comment

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

Could a named literal work better than a comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but I think it's fine that we have the comment here. At some point, we will do another round of refactoring here, but it will be much easier to do now.

@TIHan TIHan merged commit 1812755 into dotnet:main Nov 25, 2020
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
* Initial incremental build refactor

* Get previous slot

* Fixed build

* Fixed timestamp max

* Updating surface area

* Fixing last test

* Just return true

* Add minor TODO

* Change value name to something more coherent

* Fixing test
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