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

SymbolMatcher Support #44256

Merged
4 commits merged into from
May 19, 2020
Merged

SymbolMatcher Support #44256

4 commits merged into from
May 19, 2020

Conversation

333fred
Copy link
Member

@333fred 333fred commented May 14, 2020 •
edited by jcouv
Loading

Adds support for function pointers to the SymbolMatcher.

Relates to #43321 (test plan for function pointers)

@333fred 333fred requested a review from a team as a code owner May 14, 2020 17:02
@333fred 333fred requested a review from tmat May 14, 2020 17:02
@333fred
Copy link
Member Author

333fred commented May 14, 2020

@tmat for review.

tmat
tmat approved these changes May 14, 2020
View reviewed changes
Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

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

:shipit:

@333fred
Copy link
Member Author

333fred commented May 15, 2020

@dotnet/roslyn-compiler for a second review.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented May 15, 2020 •
edited by 333fred
Loading 

        public override Symbol VisitPointerType(PointerTypeSymbol symbol)

Please check if there are other interesting overrides of this API indicating that function pointers need special handling as well. #Resolved

Refers to: src/Compilers/CSharp/Portable/Emitter/EditAndContinue/CSharpSymbolMatcher.cs:552 in e17bc1c. [](commit_id = e17bc1c, deletion_comment = False)

AlekseyTs
AlekseyTs reviewed May 15, 2020
View reviewed changes
src/Compilers/CSharp/Portable/Emitter/EditAndContinue/CSharpSymbolMatcher.cs Outdated
case RefKind.Out when allowOut:
return modifiers.Length == 1;
default:
return modifiers.IsDefaultOrEmpty;
Copy link
Contributor

@AlekseyTs AlekseyTs May 15, 2020 •
edited by 333fred
Loading

Choose a reason for hiding this comment

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

IsDefaultOrEmpty [](start = 45, length = 16)

Can this be default? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

No.

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

AlekseyTs
AlekseyTs approved these changes May 15, 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 3)

@333fred
Copy link
Member Author

333fred commented May 15, 2020 

        public override Symbol VisitPointerType(PointerTypeSymbol symbol)

Added to #39865.

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

Refers to: src/Compilers/CSharp/Portable/Emitter/EditAndContinue/CSharpSymbolMatcher.cs:552 in e17bc1c. [](commit_id = e17bc1c, deletion_comment = False)

ghost
ghost approved these changes May 15, 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

@ghost ghost merged commit 373f4fd into dotnet:features/function-pointers May 19, 2020
@333fred 333fred deleted the symbol-matcher branch May 19, 2020 18:38
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmat tmat tmat 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.
4 participants
@333fred @AlekseyTs @tmat @cston