File renamings and code cleanup #357

dsyme · 2015-04-10T11:10:06Z

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.)

… master-cleanup-1

dsyme · 2015-04-10T11:10:38Z

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 · 2015-04-10T11:13:27Z

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

👍

forki · 2015-04-10T11:14:53Z

src/fsharp/AugmentWithHashCompare.fs

@@ -395,7 +396,7 @@ let mkUnionCompareWithComparer g tcref (tycon:Tycon) (_thisv,thise) (thatv,thate
    let expr = 
        let mbuilder = new MatchBuilder(NoSequencePointAtInvisibleBinding,m ) 
        let mkCase ucase =
-            let cref = mkNestedUnionCaseRef tcref ucase 
+            let cref = tcref.NestedUnionCaseRef ucase 


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

Changed to Make*

forki · 2015-04-10T11:34:36Z

src/fsharp/CompileOps.fs

        { mspec=mspec; 
          compileTimeWorkingDir=tcConfig.implicitIncludeDir;
          usesQuotations = ccu.UsesFSharp20PlusQuotations }
 #endif // NO_COMPILER_BACKEND

 let GetOptimizationData (file, ilScopeRef, ilModule, byteReader) = 
-    unpickleObjWithDanglingCcus file ilScopeRef ilModule Opt.u_LazyModuleInfo (byteReader())
+    unpickleObjWithDanglingCcus file ilScopeRef ilModule Optimizer.u_CcuOptimizationInfo (byteReader())


is this the same thing?

Yes,

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

forki · 2015-04-10T14:27:51Z

src/fsharp/vs/service.fs

@@ -743,7 +743,7 @@ type TypeCheckInfo
    let GetClassOrRecordFieldsEnvironmentLookupResolutions(line,colAtEndOfNamesAndResidue, plid, (_residue : string option)) = 
        let cursorPos = Pos.fromVS line colAtEndOfNamesAndResidue
        let (nenv, ad),m = GetBestEnvForPos cursorPos
-        let items = Nameres.ResolvePartialLongIdentToClassOrRecdFields ncenv nenv m ad plid false
+        let items = NameResolution.ResolvePartialLongIdentToClassOrRecdFields ncenv nenv m ad plid false


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

"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 · 2015-04-10T14:31:46Z

src/fsharp/tast.fs

@@ -3109,7 +3398,7 @@ and CcuResolutionResult =
    | UnresolvedCcu of string

 /// Represents the information saved in the assembly signature data resource for an F# assembly
-and PickledModuleInfo =
+and PickledCcuInfo =


What's Ccu?

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

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

Full acronym is "code compilation unit" IIRC

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

latkin · 2015-04-10T15:21:11Z

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 · 2015-04-10T15:27:32Z

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 · 2015-04-10T15:28:07Z

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

KevinRansom · 2015-04-10T17:27:05Z

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 · 2015-04-10T17:28:43Z

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 · 2015-04-10T17:51:05Z

@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 · 2015-04-10T17:54:51Z

btw good work on cleanup!! 😄

dsyme · 2015-04-11T01:20:08Z

@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 · 2015-04-11T01:26:27Z

@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 · 2015-04-13T10:04:21Z

@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 · 2015-04-17T21:43:25Z

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 · 2015-07-09T15:44:19Z

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 · 2015-08-01T01:26:49Z

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

dsyme · 2015-08-03T13:10:40Z

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

This is a pass of cleanup 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.) closes #357 commit 5e69236 Author: Don Syme <donsyme@fastmail.fm> Date: Mon Aug 3 14:09:33 2015 +0100 restore code that should not have been removed commit 7e8eda59c2929b2cfc55f24a7c96cec28994e892 Author: latkin <latkin@microsoft.com> Date: Fri Jul 31 18:15:49 2015 -0700 Fix expected text in Watson test commit 248a14c Merge: 4767d5b dd8252e Author: Don Syme <donsyme@fastmail.fm> Date: Thu Jul 9 16:42:28 2015 +0100 integrate & merge with latest HEAD commit 4767d5b Author: Don Syme <donsyme@fastmail.fm> Date: Fri Apr 10 17:25:23 2015 +0200 update to force appveyor commit 0600f3e Author: Don Syme <donsyme@fastmail.fm> Date: Fri Apr 10 16:52:21 2015 +0200 update to force appveyor commit 02c6c6c Author: Don Syme <donsyme@fastmail.fm> Date: Fri Apr 10 16:38:12 2015 +0200 update to fix build commit 877a1d2 Author: Don Syme <donsyme@fastmail.fm> Date: Fri Apr 10 16:30:50 2015 +0200 update to fix build commit dd886be Author: Don Syme <donsyme@fastmail.fm> Date: Fri Apr 10 14:03:02 2015 +0200 update to fix build commit 4f73a2b Author: Don Syme <donsyme@fastmail.fm> Date: Fri Apr 10 13:46:02 2015 +0200 update proto (4) commit 5430936 Author: Don Syme <donsyme@fastmail.fm> Date: Fri Apr 10 13:44:10 2015 +0200 update to fix build commit 93d94c9 Author: Don Syme <donsyme@fastmail.fm> Date: Fri Apr 10 13:35:15 2015 +0200 update proto () commit 77fa7ac Author: Don Syme <donsyme@fastmail.fm> Date: Fri Apr 10 13:31:35 2015 +0200 update proto and renamings commit 8797a81 Author: Don Syme <donsyme@fastmail.fm> Date: Fri Apr 10 13:05:09 2015 +0200 integrate cleanup with fsharp4 (2) commit 31777c7 Merge: c6ffdb6 bb09bb3 Author: Don Syme <donsyme@fastmail.fm> Date: Fri Apr 10 12:43:00 2015 +0200 integrate cleanup with fsharp4 commit bb09bb3 Author: Don Syme <dsyme@microsoft.com> Date: Mon Dec 1 09:53:04 2014 +0000 remove more semicolons in ilwrite.fs commit a3ca155 Author: Don Syme <dsyme@microsoft.com> Date: Sun Nov 30 20:23:14 2014 +0000 code cleanup inn ilwrite.fs and il.fs commit f2e301e Author: Don Syme <dsyme@microsoft.com> Date: Sun Nov 30 15:54:21 2014 +0000 cleanup and rename build.fs and fscopts.fs commit 20cb9b0 Author: Don Syme <dsyme@microsoft.com> Date: Sun Nov 30 00:19:12 2014 +0000 make some more functions into members in tast.fs commit ddadb30 Author: Don Syme <dsyme@microsoft.com> Date: Sat Nov 29 23:31:17 2014 +0000 additional cleanup in tast.fs (2) commit 662d87c Author: Don Syme <dsyme@microsoft.com> Date: Sat Nov 29 23:25:14 2014 +0000 additional cleanup and comments in tast.fs commit a27f527 Merge: 79b8293 4f94347 Author: Don Syme <dsyme@microsoft.com> Date: Sat Nov 29 20:37:10 2014 +0000 Merge branch 'master' of https://git01.codeplex.com/visualfsharp into master-cleanup-1 commit 79b8293 Author: Don Syme <dsyme@microsoft.com> Date: Sat Nov 29 20:35:26 2014 +0000 code cleanup and file rename

latkin · 2015-08-04T05:30:24Z

Applied to the OOB branch.

dsyme · 2015-08-04T09:17:12Z

Yay!

forki · 2015-08-04T09:18:24Z

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)
.

dsyme and others added 10 commits November 29, 2014 20:35

code cleanup and file rename

79b8293

Merge branch 'master' of https://git01.codeplex.com/visualfsharp into…

a27f527

… master-cleanup-1

additional cleanup and comments in tast.fs

662d87c

additional cleanup in tast.fs (2)

ddadb30

make some more functions into members in tast.fs

20cb9b0

cleanup and rename build.fs and fscopts.fs

f2e301e

code cleanup inn ilwrite.fs and il.fs

a3ca155

remove more semicolons in ilwrite.fs

bb09bb3

integrate cleanup with fsharp4

31777c7

integrate cleanup with fsharp4 (2)

8797a81

msftclas added the cla-not-required label Apr 10, 2015

forki reviewed Apr 10, 2015
View reviewed changes

update proto and renamings

77fa7ac

forki reviewed Apr 10, 2015
View reviewed changes

dsyme added 4 commits April 10, 2015 13:35

update proto ()

93d94c9

update to fix build

5430936

update proto (4)

4f73a2b

update to fix build

dd886be

forki reviewed Apr 10, 2015
View reviewed changes

update to fix build

877a1d2

forki reviewed Apr 10, 2015
View reviewed changes

dsyme added 2 commits April 10, 2015 16:38

update to fix build

02c6c6c

update to force appveyor

0600f3e

update to force appveyor

4767d5b

KevinRansom added the V.Next label Apr 17, 2015

KevinRansom mentioned this pull request May 6, 2015

more appropriate on conditional #399

Closed

dungpa mentioned this pull request May 19, 2015

Now we are close to shipping F# 4.0, what could we have done better #451

Closed

integrate & merge with latest HEAD

248a14c

restore code that should not have been removed

5e69236

latkin closed this Aug 4, 2015

dsyme deleted the cleanup-5 branch August 4, 2015 11:02

dsyme restored the cleanup-5 branch August 4, 2015 11:02

latkin added fixed and removed V.Next labels Aug 13, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

File renamings and code cleanup #357

File renamings and code cleanup #357

dsyme commented Apr 10, 2015

dsyme commented Apr 10, 2015

forki commented Apr 10, 2015

forki Apr 10, 2015

dsyme Apr 10, 2015

forki Apr 10, 2015

dsyme Apr 10, 2015

forki Apr 10, 2015

dsyme Apr 10, 2015

forki Apr 10, 2015

dsyme Apr 10, 2015

forki Apr 10, 2015

latkin Apr 10, 2015

enricosada Apr 11, 2015

latkin commented Apr 10, 2015

dsyme commented Apr 10, 2015

dsyme commented Apr 10, 2015

KevinRansom commented Apr 10, 2015

forki commented Apr 10, 2015

enricosada commented Apr 10, 2015

enricosada commented Apr 10, 2015

dsyme commented Apr 11, 2015

dsyme commented Apr 11, 2015

enricosada commented Apr 13, 2015

KevinRansom commented Apr 17, 2015

dsyme commented Jul 9, 2015

latkin commented Aug 1, 2015

dsyme commented Aug 3, 2015

latkin commented Aug 4, 2015

dsyme commented Aug 4, 2015

forki commented Aug 4, 2015

File renamings and code cleanup #357

File renamings and code cleanup #357

Conversation

dsyme commented Apr 10, 2015

dsyme commented Apr 10, 2015

forki commented Apr 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

latkin commented Apr 10, 2015

dsyme commented Apr 10, 2015

dsyme commented Apr 10, 2015

KevinRansom commented Apr 10, 2015

forki commented Apr 10, 2015

enricosada commented Apr 10, 2015

enricosada commented Apr 10, 2015

dsyme commented Apr 11, 2015

dsyme commented Apr 11, 2015

enricosada commented Apr 13, 2015

KevinRansom commented Apr 17, 2015

dsyme commented Jul 9, 2015

latkin commented Aug 1, 2015

dsyme commented Aug 3, 2015

latkin commented Aug 4, 2015

dsyme commented Aug 4, 2015

forki commented Aug 4, 2015