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

Support symbol retargeting for function pointers. #43625

Merged
3 commits merged into from
May 2, 2020
Merged

Support symbol retargeting for function pointers. #43625

3 commits merged into from
May 2, 2020

Conversation

333fred
Copy link
Member

@333fred 333fred commented Apr 23, 2020

@AlekseyTs @dotnet/roslyn-compiler for review.

@333fred 333fred requested review from AlekseyTs and a team April 23, 2020 23:21
@333fred 333fred mentioned this pull request Apr 23, 2020
Closed
18 tasks
@333fred
Copy link
Member Author

333fred commented Apr 23, 2020 •
edited
Loading

Note that there was no PROTOTYPE comment associated with this, it had already been converted to a bullet in #39865 #Resolved

@333fred
Copy link
Member Author

333fred commented Apr 24, 2020

@AlekseyTs @dotnet/roslyn-compiler for a review.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Apr 24, 2020 •
edited
Loading

Done with review pass (iteration 1) #Closed

@333fred
Copy link
Member Author

333fred commented Apr 29, 2020

@AlekseyTs addressed feedback. I totally rewrote the tests in RetargetingTests.cs (and that's the only change made), so I'd suggest not looking at the diff between iteration 1 and 2 and just reviewing RetargetingTests.cs from scratch as the diff is a bit messy.

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

AlekseyTs
AlekseyTs reviewed Apr 30, 2020
View reviewed changes
src/Compilers/CSharp/Test/Symbol/Symbols/Retargeting/RetargetingTests.cs
@@ -750,6 +750,250 @@ class C1<T>
Assert.Equal(c1.MangleName, c1r.MangleName);
Assert.Equal(c1.MetadataName, c1r.MetadataName);
}

[Fact]
public void FunctionPointerRetargeting_FullyConsistent()
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 30, 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.

public [](start = 8, length = 6)

[CompilerTrait(CompilerFeature....)]? And for other new tests #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.

I'll do this in a separate PR. I haven't added a CompilerFeature flag nor have I done this for any existing tests. I'd want to either do everything or nothing.

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

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Apr 30, 2020 •
edited
Loading

Done with review pass (iteration 2) #Closed

333fred
333fred commented Apr 30, 2020
View reviewed changes
src/Compilers/CSharp/Test/Symbol/Symbols/Retargeting/RetargetingTests.cs
var retargeted2 = CreateCompilation(retargetedIdentity.WithVersion(new Version(2, 0, 0, 0)), new[] { retargetedSource }, references: standardReference);
var retargeted2Ref = retargeted2.ToMetadataReference();

var consistent = CreateCompilation("public class C {}", assemblyName: "Con", targetFramework: TargetFramework.Standard);
Copy link
Member Author

Choose a reason for hiding this comment

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

Because I had to add an explicit StandardReference above (because that CreateCompilation overload does not add default references), I'm being explicit in the others rather than relying on any change to CreateCompilation to change for all of the calls.

@333fred
Copy link
Member Author

333fred commented Apr 30, 2020

@AlekseyTs addressed feedback.

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

AlekseyTs
AlekseyTs approved these changes Apr 30, 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 Apr 30, 2020

@dotnet/roslyn-compiler for a second review.

chsienki
chsienki approved these changes May 1, 2020
View reviewed changes
Copy link
Contributor

@chsienki chsienki 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 1, 2020

Rebased on top of the latest master-merge

ghost
ghost approved these changes May 2, 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 f339de3 into dotnet:features/function-pointers May 2, 2020
@333fred 333fred deleted the retargeting branch May 2, 2020 15:50
@jinujoseph jinujoseph added this to the Next milestone May 3, 2020
@JoeRobich JoeRobich modified the milestones: Next, 16.7.P2 May 18, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaredpar jaredpar jaredpar left review comments

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@chsienki chsienki chsienki approved these changes

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

Successfully merging this pull request may close these issues.
6 participants
@333fred @AlekseyTs @jaredpar @chsienki @JoeRobich @jinujoseph