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

[CompilerPerf] Pretify ConstrainsSolver method #5141

Merged
merged 2 commits into from
Jun 12, 2018

Conversation

realvictorprm
Copy link
Contributor

The method wasn't readable at all before now it's better.

Converted the code to use an computation expression.

The method wasn't readable at all before now it's better.
@dsyme
Copy link
Contributor

dsyme commented Jun 8, 2018

I'm open to converting the whole of ConstraintSolver.fs to use trackErrors subject to checking that performance is roughly equivalent. It would hugely improve readability. Let's do it step by step

One note: we may need to add a Delay method to trackErrors to give accurate semantics, especially if a TryWith and is also added to replace uses of TryD

@dsyme dsyme changed the title [WIP] Pretify ConstrainsSolver method [CompilerPerf] [WIP] Pretify ConstrainsSolver method Jun 8, 2018


let calledObjArgTys = calledMeth.CalledObjArgTys(m)
if minst.Length <> uminst.Length then
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this outside the trackErrors?

do! calledMeth.ArgSets
|> IterateD (fun argSet ->
argSet.AssignedNamedArgs |> IterateD (fun arg -> subsumeArg arg.CalledArg arg.CallerArg))
do! assignedItemSetters
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use a for loop here, since trackErrors has a For method?

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 need to implement more methods so the for do is usable.

|> IterateD (fun argSet ->
if argSet.UnnamedCalledArgs.Length <> argSet.UnnamedCallerArgs.Length then ErrorD(Error(FSComp.SR.csArgumentLengthMismatch(), m)) else
Iterate2D subsumeArg argSet.UnnamedCalledArgs argSet.UnnamedCallerArgs)
do! calledMeth.ParamArrayCalledArgOpt
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be best to use

    match calledMeth.ParamArrayCalledArgOpt with 
    | None -> ()
    | Some calledArg -> 
        ...

trackErrors {
do! Iterate2D unifyTypes minst uminst
do!
if not (permitOptArgs || isNil unnamedCalledOptArgs) then
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you don't need the do!, you can go straight into the if and then return! on each branch

        trackErrors {
            do! Iterate2D unifyTypes minst uminst
            if not (permitOptArgs || isNil unnamedCalledOptArgs) then 
                return! ErrorD(Error(FSComp.SR.csOptionalArgumentNotPermittedHere(), m)) 
            else
             ....

@realvictorprm realvictorprm changed the title [CompilerPerf] [WIP] Pretify ConstrainsSolver method Pretify ConstrainsSolver method Jun 8, 2018
@realvictorprm
Copy link
Contributor Author

cool ok you can see on master...realvictorprm:fix-3814 that I already started implementing required methods so for .. do can be used :)

Signed-off-by: realvictorprm <mueller.vpr@gmail.com>
@realvictorprm
Copy link
Contributor Author

Is this the right direction @dsyme?

@realvictorprm
Copy link
Contributor Author

@dotnet-bot test this please

@dsyme
Copy link
Contributor

dsyme commented Jun 11, 2018

@dotnet-bot test Ubuntu16.04 Release_fcs Build please

@dsyme dsyme changed the title Pretify ConstrainsSolver method [CompilerPerf] Pretify ConstrainsSolver method Jun 11, 2018
@KevinRansom KevinRansom merged commit 6f8f5f2 into dotnet:master Jun 12, 2018
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