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

@krauthaufenkrauthaufen commented Aug 17, 2017

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 reviewed Aug 17, 2017
View reviewed changes

@forkiforki 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

krauthaufen commented Aug 17, 2017

Copy link
Copy Markdown
ContributorAuthor

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 requested changes Aug 17, 2017
View reviewed changes

@dsymedsyme 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

KevinRansom commented Aug 29, 2017

Copy link
Copy Markdown
Contributor

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

@dotnetdotnet deleted a comment from msftclasSep 26, 2017
@dsymedsyme mentioned this pull request Oct 4, 2017
Closed
@dsyme

dsyme commented Oct 4, 2017

Copy link
Copy Markdown
Contributor

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

@krauthaufen

krauthaufen commented Oct 4, 2017

Copy link
Copy Markdown
ContributorAuthor

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?

@krauthaufen
[Test] added tests for AddressOf quotations
6706929
- 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•
edited
Loading

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@krauthaufen

krauthaufen commented Oct 5, 2017

Copy link
Copy Markdown
ContributorAuthor

@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:masterOct 17, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dsymedsymedsyme approved these changes
+2 more reviewers
@forkiforkiforki left review comments
@KevinRansomKevinRansomKevinRansom approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@krauthaufen@dsyme@KevinRansom@msftclas@forki@msftgits