Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Public API support for function pointer types #44436

Merged
merged 13 commits into from
May 28, 2020
Merged

Add Public API support for function pointer types #44436

merged 13 commits into from
May 28, 2020

Conversation

333fred
Copy link
Member

@333fred 333fred commented May 20, 2020

Implements and documents the public API access points for function pointer types.

@333fred 333fred force-pushed the public-api branch 6 times, most recently from cf96a95 to b9d7cad Compare May 20, 2020 20:42
@333fred 333fred requested review from AlekseyTs and a team May 20, 2020 23:06
@333fred 333fred marked this pull request as ready for review May 20, 2020 23:06
@333fred
Copy link
Member Author

333fred commented May 20, 2020 •
edited
Loading

@AlekseyTs @dotnet/roslyn-compiler for review. /cc @jasonmalinowski @CyrusNajmabadi, I've added the CreateFunctionPointerTypeSymbol api in this change. It doesn't have calling convention yet, as we don't have a public API for that yet either. #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented May 27, 2020 •
edited
Loading

Done with review pass (iteration 8) #Closed

@333fred
Copy link
Member Author

333fred commented May 27, 2020

@AlekseyTs addressed feedback.

In reply to: 634421183 [](ancestors = 634421183)

AlekseyTs
AlekseyTs approved these changes May 27, 2020
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (iteration 9), assuming CI is passing

@cston
Copy link
Member

cston commented May 27, 2020 •
edited
Loading 

            Optional<object> constantValue = ConvertToOptional(boundConversion.ConstantValue);

Consider moving these ahead of if (boundConversion is ...) so these values can be used in that if body as well. #Closed

Refers to: src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs:960 in 1edbf72. [](commit_id = 1edbf72, deletion_comment = False)

ghost
ghost approved these changes May 27, 2020
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Auto-approval

jasonmalinowski
jasonmalinowski approved these changes May 27, 2020
View reviewed changes
src/Compilers/Core/Portable/Compilation/Compilation.cs
Comment on lines +954 to +955
/// If returnType is <see langword="null"/>, or if parameterTypes or parameterRefKinds are default,
/// or if any of the types in parameterTypes are null.</exception>
Copy link
Member

@jasonmalinowski jasonmalinowski May 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The expectation is the return type is System.Void for void-returning? I ask only because System.Void is...funky and it's use is sometimes unclear. 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep.

@333fred 333fred merged commit b8cb1f8 into dotnet:features/function-pointers May 28, 2020
@333fred 333fred deleted the public-api branch May 28, 2020 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@jasonmalinowski jasonmalinowski jasonmalinowski approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@cston cston cston approved these changes

Assignees
No one assigned
Labels
Area-Compilers auto-merge Feature - Function Pointers Adding Function Pointers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@333fred @AlekseyTs @cston @jasonmalinowski @CyrusNajmabadi