Skip to content

Conversation

@dsyme
Copy link
Contributor

@dsyme dsyme commented Apr 2, 2020

Replacement for #8865 coming from a different branch

The changes are cosmetic. The one change to values is in IlxGen.fs where it is cleaner to use a list for the "selfv" rather than an option in some places and a list in another. But that is also a cosmetic change as it's easy to verify that the list always contains 0 or 1 elements here.

There is also some improvement to debugging in the compiler with much improved DebugText for Expr and TOp values. THis helps speed debugger performance and I think prevents some stack overflow crashed suring debugging when formatting large expressions.

Also trim length of names of tests in FSharp.Core.UnitTests.

@TIHan
Copy link
Contributor

TIHan commented Apr 2, 2020

@dsyme looking at it

@dsyme
Copy link
Contributor Author

dsyme commented Apr 2, 2020

@dsyme looking at it

No big rush, @cartermp explained you've been dealing with the regressions :)

@dsyme
Copy link
Contributor Author

dsyme commented Apr 2, 2020

Also, I learned the trick of pushing cleanup associated with a feature

feature/tasks

to

feature/tasks-cleanup

and then pointing feature/tasks --> feature/tasks-cleanup in the RFC PR. Then the minimized diff shows in that PR, which is really useful for everyone. Once the cleanup gets integrated I can repoint the RFC PR at master.

This means there is really no rush with these PR-related cleanups, an I'll use this technique more regularly now.

let expr = stripExpr expr

// Process the debug point and see if there's a replacement technique to process this expression
if GenExprPreSteps cenv cgbuf eenv sp expr sequel then () else
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just 2-char indentation apart from this line

reprL

and bindingL g (TBind(v, repr, _)) =
valAtBindL g v --- (wordL(tagText "=") ^^ exprL g repr)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are just cleanup to DebugPrint to give slightly better formatting when dumping expressions etc.

#else
[<System.Diagnostics.DebuggerDisplay("({StartLine},{StartColumn}-{EndLine},{EndColumn}) {FileName} IsSynthetic={IsSynthetic}")>]
#endif
[<System.Diagnostics.DebuggerDisplay("({StartLine},{StartColumn}-{EndLine},{EndColumn}) {ShortFileName} -> {DebugCode}")>]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no need to make this debugging support differ in DEBUG mode


#r @"..\..\..\..\Release\net40\bin\FSharp.Core.UnitTests.dll"

FSharp.Core.UnitTests.FSharp_Core.Microsoft_FSharp_Core.ComparersRegression.createData ()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I shortned a lot of names since they are crazy long in the Test Explorer and there was redundancy in the names in any case

let expr = stripExpr expr
/// Process the debug point and check for alternative ways to generate this expression.
/// Returns 'true' if the expression was processed by alternative means.
and GenExprPreSteps (cenv: cenv) (cgbuf: CodeGenBuffer) eenv sp expr sequel =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This factors out these "pre steps" into a separate method, which we use from both GenExprAux and GenLinearExpr.

This factoring is useful for me in the other PR when detecting and generating code for state machines.

| None ->
false

match expr with
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This big change is nearly all 2-char indentation

Copy link
Contributor

@TIHan TIHan left a comment

Choose a reason for hiding this comment

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

Looks fine to me!

@cartermp cartermp closed this Apr 2, 2020
@cartermp cartermp reopened this Apr 2, 2020
@dsyme dsyme merged commit 2e0b611 into master Apr 3, 2020
@dsyme
Copy link
Contributor Author

dsyme commented Apr 3, 2020

Thanks all!

@brettfo brettfo deleted the feature/tasks-cleanup branch April 16, 2020 17:19
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
* cleanup to minimize diff for RFC FS-1087

* a little more cleanup

* a little more cleanup

* a little more cleanup

* trim length of names in FSHarp.Core.UnitTests

* min diff

* min diff
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.

4 participants