Skip to content

Conversation

@edgarfgp
Copy link
Contributor

While contributing I always find confusing using custom operators.

In this PR my goal is to replace the usage of

val (++): res: OperationResult<'T> -> f: ('T -> OperationResult<'b>) -> OperationResult<'b>`

with

trackErrors {
    ....
}

@edgarfgp
Copy link
Contributor Author

edgarfgp commented Aug 15, 2023

I am not entirely sure how to convert the last one.

TryD (fun () -> SolveTypeSubsumesType csenv 0 m NoTrace None reqdObjTy availObjTy ++ (fun () -> ResultD true))
                 (fun _err -> ResultD false)

https://github.com/dotnet/fsharp/blob/0514e881c2e03b2c004504b8b21432aa4a01c7bc/src/Compiler/Checking/ConstraintSolver.fs#L3769C94-L3769C100

Update: This is now done

@edgarfgp edgarfgp marked this pull request as ready for review August 16, 2023 08:06
@edgarfgp edgarfgp requested a review from a team as a code owner August 16, 2023 08:06
@edgarfgp edgarfgp requested a review from kerams August 16, 2023 08:15
@vzarytovskii
Copy link
Member

That's good, we can probably remove the (++) if it's not used anywhere. I find its usage confusing and not useful. Thanks for rewriting it.

@edgarfgp
Copy link
Contributor Author

edgarfgp commented Aug 16, 2023

That's good, we can probably remove the (++) if it's not used anywhere. I find its usage confusing and not useful. Thanks for rewriting it.

(++) is now only used in DiagnosticsLogger TrackErrorsBuilder and in some of its helper functions. So I guess We can rewrite it and remove the usage of the custom operator In a separate PR ?

Update: See

type TrackErrorsBuilder() =

Copy link
Contributor

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Thanks for this.

As for removing (++) completely - as usual, I'd prefer a separate PR but not opposed to having it here either :)

@edgarfgp edgarfgp requested a review from vzarytovskii August 16, 2023 20:18
@kerams
Copy link
Contributor

kerams commented Aug 23, 2023

I'm looking back at this, there are a bunch of do! CompleteD. Isn't that essentially a no-op?

@edgarfgp
Copy link
Contributor Author

I'm looking back at this, there are a bunch of do! CompleteD. Isn't that essentially a no-op?

Do you mean that these can go outside of the trackErrors ce ?

@kerams
Copy link
Contributor

kerams commented Aug 23, 2023

Does it do anything? It seems to me it could be replaced with () and some branches could be eliminated altogether.

@edgarfgp
Copy link
Contributor Author

Does it do anything? It seems to me it could be replaced with () and some branches could be eliminated altogether.

Yeah that's a good point. On this PR I wanted to just replace the ++ operator, But I think it can be optimized in follow-up PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants