Skip to content

Conversation

@psfinaki
Copy link
Contributor

@psfinaki psfinaki commented Aug 9, 2022

No description provided.

@psfinaki psfinaki requested a review from dsyme August 9, 2022 15:30
@psfinaki psfinaki linked an issue Aug 9, 2022 that may be closed by this pull request
@psfinaki psfinaki marked this pull request as ready for review August 10, 2022 17:37
@dsyme
Copy link
Contributor

dsyme commented Aug 11, 2022

@psfinaki Basically this looks good, though I'd like to do some more on it next week to reap more benefit from the changes.

I pushed some adjustments

  • Rename CheckTypes.fs/fsi to CheckBasics.fs/fsi. We will use "CheckTypes.fs/fsi" for TcType and friends.

  • I moved one thing back to CheckExpressions.fs since it wasn't specifically needed in CheckBasics.fs (the purpose of CheckBasics should be to establish TcEnv and TcFileState and nothing else).

  • I'd imagine we will move most of the helpers in CheckExpressions.fs which only relate to TcEnv and TcFileState and the other such types into CheckBasics.fs. I pushed one example of this - often this will involve making things into members according to the types they operate on. This is part of the benefit we can gain by this - more of the logic of CheckExpressions.fs will migrate out and be organized differently.

  • I guess we may eventually split CheckBasics itself into TcEnv.fs and TcFileState.fs though no need to do that for now.

We can merge this for now I think and get it in the bank, then iterate next week.

@dsyme
Copy link
Contributor

dsyme commented Aug 11, 2022

Just to note that, once we iterate, this lays the foundation for further splitting, e.g.

  • CheckAttributes.fs/fsi for TcAttribute* and friends
  • CheckTypes.fs/fsi for TcType*
  • GeneralizationHelpers.fs/fsi (helpers related to generalization)
  • EliminateInitializationGraphs.fs/fsi (EliminateInitializationGraphs and friends)
  • CheckIndexAndSlice.fs/fsi (TcIndexerThen* - quite a large chunk)
  • CheckObjectExpressions.fs/fsi (TcObJExpr* - quite a large chunk)
  • CheckBindings.fs/fsi (normalizing bindings, TcLet and incremental generalization of groups of recursive bindings)
  • CheckStrings.fs/fsi (checking format strings, interpolated strings)

@psfinaki psfinaki merged commit 4330fa1 into main Aug 12, 2022
@psfinaki psfinaki deleted the psfinaki/refactoring branch August 12, 2022 11:58
@psfinaki
Copy link
Contributor Author

@dsyme thanks for the review and the adjustments. Yeah here we already have quite a lot of changes, so merging this one - and hope we'll address your points above in the near future :)

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.

Chore: split monster-sized modules in the codebase

3 participants