Skip to content

Remove DelegateInvokeStubManager - #88683

Merged
AaronRobinsonMSFT merged 2 commits into
dotnet:mainfrom
AaronRobinsonMSFT:runtime_85339
Jul 13, 2023
Merged

Remove DelegateInvokeStubManager#88683
AaronRobinsonMSFT merged 2 commits into
dotnet:mainfrom
AaronRobinsonMSFT:runtime_85339

Conversation

@AaronRobinsonMSFT

@AaronRobinsonMSFTAaronRobinsonMSFT commented Jul 11, 2023•
edited
Loading

Copy link
Copy Markdown
Member

All Delegate Invoke scenarios now use the StubLinker. This means there is an inherent confusion between the two stub managers. Upon further inspection combining these two by adding a new flag on the Stub class means they can be combined logically and simplify the code path by removing a StubManager.

Fixes#85339

@AaronRobinsonMSFT
Remove DelegateInvokeStubManager
ee20cb5
All Delegate Invoke scenarios now use the StubLinker.
This means there is an inherent confusion between the
two stub managers. Upon further inspection combining
these two by adding a new flag on the Stub class means
they can be combined logically and simplify the code
path by removing a StubManager.
@AaronRobinsonMSFTAaronRobinsonMSFT added this to the 8.0.0 milestone Jul 11, 2023
@ghost

ghost commented Jul 11, 2023

Copy link
Copy Markdown

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

All Delegate Invoke scenarios now use the StubLinker. This means there is an inherent confusion between the two stub managers. Upon further inspection combining these two by adding a new flag on the Stub class means they can be combined logically and simplify the code path by removing a StubManager.

#Fixes 85339

Author:AaronRobinsonMSFT
Assignees:-
Labels:

area-Diagnostics-coreclr

Milestone:8.0.0

@AaronRobinsonMSFT

AaronRobinsonMSFT commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

/cc @dotnet/dotnet-diag

This was referenced Jul 11, 2023
Closed
Closed
hoyosjs
hoyosjs approved these changes Jul 12, 2023
View reviewed changes
Comment threadsrc/coreclr/vm/stubmgr.h Outdated
noahfalk
noahfalk approved these changes Jul 12, 2023
View reviewed changes

@noahfalknoahfalk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

This was referenced Jul 13, 2023
Closed
Closed
@AaronRobinsonMSFT

AaronRobinsonMSFT commented Jul 13, 2023

Copy link
Copy Markdown
MemberAuthor

The failures are known. I reran the tests before checking for known issues.

@AaronRobinsonMSFT
AaronRobinsonMSFT merged commit 08de046 into dotnet:mainJul 13, 2023
@AaronRobinsonMSFT
AaronRobinsonMSFT deleted the runtime_85339 branch July 13, 2023 04:20
AustinWise
AustinWise reviewed Jul 13, 2023
View reviewed changes
Comment threadsrc/coreclr/vm/stublink.h
@AaronRobinsonMSFTAaronRobinsonMSFT mentioned this pull request Jul 13, 2023
Merged
@AaronRobinsonMSFTAaronRobinsonMSFT mentioned this pull request Jul 24, 2023
Closed
@ghostghost locked as resolved and limited conversation to collaborators Aug 13, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@noahfalknoahfalknoahfalk approved these changes
@hoyosjshoyosjshoyosjs approved these changes
@janvorlijanvorliAwaiting requested review from janvorli
+1 more reviewer
@AustinWiseAustinWiseAustinWise left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@AaronRobinsonMSFTAaronRobinsonMSFT

Labels

area-Diagnostics-coreclr

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

Native->Managed (.net core) step-in not working in Visual Studio

4 participants

@AaronRobinsonMSFT@AustinWise@noahfalk@hoyosjs