File renamings and code cleanup

[dsyme]

This updates http://visualfsharp.codeplex.com/SourceControl/network/forks/dsyme/cleanup?branch=master-cleanup-1 to integrate into fsharp4.

This is a pass of cleanup suitable for the "master" branch that renames some files to get rid of most cryptic abbreviations like "csolve", and does some other code cleanup and documentation too.

• commenting in "detuple.fs"
  • removing ExtensibleDumper.fs which is an old adhoc debug mechanism barely used in the codebase
  • removing some dead code in fsc.fs (some code was already duplicated in fscmain.fs too!)
  • renaming check.{fs,fsi} --> PostInferenceChecks.{fs,fsi}
  • renaming tc.{fs,fsi} --> TypeChecker.{fs,fsi}
  • renaming opt.{fs,fsi} --> Optimizer.{fs,fsi}
  • renaming est.{fs,fsi} --> ExtensionTyping.{fs,fsi}
  • renaming build.{fs,fsi} --> CompileOps.{fs,fsi}
  • renaming fscopts.{fs,fsi} --> CompileOptions.{fs,fsi}
  • moving the option parser to CompileOptions.fs (where it belongs!)
  • marking some record types as RequireQualifiedAccess (to give better errors when editing the compiler)
  • removed a whole bunch of semicolons
  • removed some old debugging output (verboseStamps etc.)

[dsyme]

Personally I think we should integrate this sooner rather than later. We regularly integrate along the path Microsoft/visualfsharp --> fsharp/fsharp --> fsharp/FSharp.Compiler.Service for master and fsharp4 branches. The cleanup can be relatively quickly integrated across all three repos and we can move on in a new, cleaner world.

[forki]

Oh I just learned to love the cool names like tc.fs.

👍

[forki]

what was the mk? Shouldn't it be something like tcref.MakeNestedUnionCaseRef?

[dsyme]

Changed to Make*

[forki]

is this the same thing?

[dsyme]

Yes,

let p_CcuOptimizationInfo x st = p_LazyModuleInfo x st
let u_CcuOptimizationInfo st = u_LazyModuleInfo st

[forki]

Recd could be renamed to Record. It's only 2 chars more in a very long identifier

[dsyme]

"Recd" is used pretty extensively in the codebase so it's consistent. It's also kind of useful to distinguish it from the verb "Record".

[forki]

What's Ccu?

[dsyme]

Compilation unit, see tast.fs. Essentially the F# view of the logical contents of an assembly for the purposes of type checking.

[forki]

mhm. I assume this is already established, but maybe something like CU would fit better. I wonnder where the second c comes from.

[latkin]

Full acronym is "code compilation unit" IIRC

[enricosada]

Maybe a glossary with all the acronyms can help.
Easy doc and useful ( search text ccu in repo -> glossary.md )

[latkin]

We were reluctant to take the original PR as it came in at a time when there were a number of other PRs pending which would have been a pain to merge. That's no longer the case so this should be easier to accept now. Any chance you are going to migrate "cleanup referenceresolution.fs/fsi" too?

[dsyme]

AppVeyor just failed with this, which looks disturbing, as though there may still be a race condition, see RaceBetweenCancellationHandlerAndDisposingHandlerRegistration. I'm pretty certain it's unrelated to this PR but will check.

 1) Test Failure : FSharp.Core.Unittests.FSharp_Core.Microsoft_FSharp_Control.AsyncModule.OnCancel.RaceBetweenCancellationHandlerAndDisposingHandlerRegistration
      Expected: True
   But was:  False

[dsyme]

@latkin - Yes, I'll move that other one too, thanks for reminding me

[KevinRansom]

These large cleanup PR's suck when reviewed using IE, it slows down to a crawl.

I like the file renaming and field renaming, the style is more similar to .Net style.

Some of this cleanup looks like whitespace reformatting. Is that for consistency? Or is it because you prefer this style? Or perhaps an artifact of a reformat tool that you ran? My preference is to avoid large scale whitespace refactoring and personal preference refactoring?

I believe Roslyn actually runs a tool that slams all code into a particular "authorized format" (and every time I hear about it, is because of some bug or other). If we wanted to codify a set of coding guidelines, we might want to decide them and publish them. Although we do have quite a lot of other work coming up.

I'm okay with accepting this PR, but I would prefer to limit the personal preference refactoring to an agreed style moving forward. What do you think?

Kevin

[forki]

Works well in Chrome.
On Apr 10, 2015 7:27 PM, "Kevin Ransom (msft)" notifications@github.com
wrote:

These large cleanup PR's suck when reviewed using IE, it slows down to a
crawl.

I like the file renaming and field renaming, the style is more similar to
.Net style.

Some of this cleanup looks like whitespace reformatting. Is that for
consistency? Or is it because you prefer this style? Or perhaps an artifact
of a reformat tool that you ran? My preference is to avoid large scale
whitespace refactoring and personal preference refactoring?

I believe Roslyn actually runs a tool that slams all code into a
particular "authorized format" (and every time I hear about it, is because
of some bug or other). If we wanted to codify a set of coding guidelines,
we might want to decide them and publish them. Although we do have quite a
lot of other work coming up.

I'm okay with accepting this PR, but I would prefer to limit the personal
preference refactoring to an agreed style moving forward. What do you think?

Kevin

—
Reply to this email directly or view it on GitHub
#357 (comment)
.

[enricosada]

@dsyme i think you just lost some rename opportunity in git, like this you lose file history (or harder to follow).

for example in 31777c7
DELETED src/fsharp/tc.fsi
ADDED src/fsharp/TypeChecker.fsi

no rename here

maybe easier to split work in two phase (or use a more aggressive rename detection):

• rename files + change fsproj
• refactor content

[enricosada]

btw good work on cleanup!! 😄

[dsyme]

@enricosada Do you know if I can get more aggressive rename detection applied by any technique? e.g by resubmitting the PR, or squashing etc? Thanks

[dsyme]

@KevinRansom All the changes are in some sense moving towards a more normalized style across files or across the compiler codebase as a whole - consistency is what counts here. There are still plenty of inconsistencies but I think we can work on that bit by bit.

I'll aim to write up the conventions used as a set of guidelines, and perhaps also a set of settings for VFPT code formatting (which I haven't used here though it would make a marvellous stress test for the code formatter :) )

btw yes, use Chrome to view this PR :)

[enricosada]

@dsyme so, git does not track rename in commit, it does check for rename in diff or merge command.
So you dont lose history (my bad) but is difficult to merge usually or see diff (you need to increase threshold when -follow-rename).

So:

• the user increase the threshold of rename-detection in diff/merge (command line args or config) when needed
• you can help git splitting rename+modify. see splitting commits with interactive rebase

if you need help, ping me, i use rebase alot (i like to cleanup and split before pull request).

I think easier workflow is

• git rebase -i
  • fixup last commits (the fix build), less commit to manage
• git rebase -i
  • edit 31777c7
  • commit all good renames
  • commit deleted and the new rename (= deleted)
  • commit modify
  • rebase --continue

[KevinRansom]

Thank you for taking the time to take care of this work, we will take care of this at the start of the next cycle.

Moving this to V.Next per: #371

[dsyme]

I've updated this with a merge against the latest head of fsharp4. Let me know if you'd like me to send it to another branch. It would be great to get these clean-ups in :)

[latkin]

Almost ready to apply this. One issue found by tests: dsyme@20cb9b0#commitcomment-12477580

[dsyme]

OK, it looks like one of those cases should have been deleted but not both. Have fixed that now with 5e69236

[latkin]

Applied to the OOB branch.

[dsyme]

Yay!

[forki]

So should we rebase our PRs on this? Would probably make things easier for
@latkin.
On Aug 4, 2015 11:17, "Don Syme" notifications@github.com wrote:

Yay!

—
Reply to this email directly or view it on GitHub
#357 (comment)
.