Skip to content

typeof in quotations.fs now respects ByRef types#3450

Merged
KevinRansom merged 2 commits into
dotnet:masterfrom
krauthaufen:master
Oct 17, 2017
Merged

typeof in quotations.fs now respects ByRef types#3450
KevinRansom merged 2 commits into
dotnet:masterfrom
krauthaufen:master

Conversation

@krauthaufen

Copy link
Copy Markdown
Contributor

When using AddressOf in quotations typeof function simple threw an exception which makes it impossible to call methods using byref-arguments.

Since there is a .NET-representation for byref-types why not use it?

@forki forki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't the remaining case still give that specific error message?

@krauthaufen

Copy link
Copy Markdown
Contributor Author

Shouldn't AddressOf contain one sub-expression only?

@dsyme

dsyme commented Aug 17, 2017

Copy link
Copy Markdown
Contributor

@krauthaufen Could you add a test too please? See src\fsharp\FSharp.Core.Unittests

@dsyme dsyme left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

requested a test be added

@KevinRansom

Copy link
Copy Markdown
Contributor

@krauthaufen Hi, thanks for this PR, did you notice Don's comment?

@dsyme

dsyme commented Oct 4, 2017

Copy link
Copy Markdown
Contributor

@krauthaufen could you add a test for this please? Many thanks

@krauthaufen

Copy link
Copy Markdown
Contributor Author

Hi, sorry for the late response. i will look into it tomorrow.

@dsyme

dsyme commented Oct 4, 2017

Copy link
Copy Markdown
Contributor

@krauthaufen Great! See tests\fsharp\core\quotes\test.fsx?

- AddressOf now has a proper type (byref<'a>)
- AddressOf can be used in Expr.Call (was an error)
- calls containing AddressOf can be rebuilt using RebuildShapeCombination
@msftclas

msftclas commented Oct 5, 2017

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@krauthaufen

Copy link
Copy Markdown
Contributor Author

@dsyme I just added a few tests checking that AddressOf can now be used as argument to Expr.Call(...), etc.
This was failing due to the exception in typeof.
Couldn't really think of more cases where this change affects anything :-)

@KevinRansom KevinRansom merged commit e5762fd into dotnet:master Oct 17, 2017
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.

6 participants