Skip to content

Conversation

@latkin
Copy link
Contributor

@latkin latkin commented Jan 30, 2016

Fixes #907

Aligns F# compiler's treatment of these optional arguments with what C# and VB do.

For omitted obj optional args:

  • If it has MarshalAs(IUnknown), MarshalAs(IDispatch), or MarshalAs(Interface), emit a null default value
  • Else if it has IUnknownConstantAttribute emit UnknownWrapper(null)
  • Else if it has IDispatchConstantAttribute emit DispatchWrapper(null)
  • Else emit Type.Missing

See relevant bits of the C# compiler and VB compiler.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why a function? Ti can ve replaced with the function body

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #907 (comment) - extra problems occur when you put the code in a function. Wanted to make sure that was covered.

@msftclas
Copy link

@latkin, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

@msftclas
Copy link

msftclas commented Feb 2, 2016

@latkin, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

@dsyme
Copy link
Contributor

dsyme commented Feb 2, 2016

OK, looks good!

@enricosada
Copy link
Contributor

@dotnet-bot test this please

KevinRansom added a commit that referenced this pull request Feb 25, 2016
Fix handling of optional IDispatch, IUnknown method args
@KevinRansom KevinRansom merged commit ba68a23 into dotnet:master Feb 25, 2016
@KevinRansom
Copy link
Contributor

Thanks for taking care of this Lincoln, we miss you, and hope you are having a great time in your new Job.

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.

5 participants