Skip to content

Conversation

@forki
Copy link
Contributor

@forki forki commented May 29, 2020

No description provided.

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

One small change still needed, otherwise this looks fine to me

let wrap, h', _readonly, _writeonly = mkExprAddrOfExpr g true false PossiblyMutates h None m
Some (wrap (Expr.Op (TOp.TraitCall traitInfo, [], (h' :: t), m)))
if minfo.IsStruct && minfo.IsInstance then
match argExprs with
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't quite the same, it's missing the [] -> false case.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cartermp , I believe that case is taken care of in isByRefTy but boolean logic has always been my downfall.

let isByrefTy g ty = 
    ty |> stripTyEqns g |> (function 
        | TType_app(tcref, _) when g.byref2_tcr.CanDeref -> tyconRefEq g g.byref2_tcr tcref
        | TType_app(tcref, _) -> tyconRefEq g g.byref_tcr tcref
        | _ -> false) 

Copy link
Contributor

Choose a reason for hiding this comment

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

I also suffer from boolean logic, which is also why I think it'd be better if the original [] -> false case was restored

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cartermp no the [] case is still there. It's just rearranged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe more correct: one of the two tests for [] is still in place

Copy link
Contributor

Choose a reason for hiding this comment

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

It is, I see it now, I was always hopeless at where's waldo.

Copy link
Contributor

Choose a reason for hiding this comment

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

aha, I think I see it now too. The original code was so hard to read

let wrap, h', _readonly, _writeonly = mkExprAddrOfExpr g true false PossiblyMutates h None m
Some (wrap (Expr.Op (TOp.TraitCall traitInfo, [], (h' :: t), m)))
if minfo.IsStruct && minfo.IsInstance then
match argExprs with
Copy link
Contributor

Choose a reason for hiding this comment

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

@cartermp , I believe that case is taken care of in isByRefTy but boolean logic has always been my downfall.

let isByrefTy g ty = 
    ty |> stripTyEqns g |> (function 
        | TType_app(tcref, _) when g.byref2_tcr.CanDeref -> tyconRefEq g g.byref2_tcr tcref
        | TType_app(tcref, _) -> tyconRefEq g g.byref_tcr tcref
        | _ -> false) 

@cartermp cartermp merged commit 4f0d6bf into dotnet:master May 29, 2020
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
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