Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Allow supressing exceptions in diamond inheritance cases#20458

Merged
MichalStrehovsky merged 4 commits intodotnet:masterfrom
MichalStrehovsky:fix16123
Nov 15, 2018
Merged

Allow supressing exceptions in diamond inheritance cases#20458
MichalStrehovsky merged 4 commits intodotnet:masterfrom
MichalStrehovsky:fix16123

Conversation

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky MichalStrehovsky commented Oct 17, 2018

This is an attempt to add a flag that allows suppressing exceptions in diamond inheritance case of default interface methods so that we don't have to wrap things in a try/catch.

Fixes #16123

@MichalStrehovsky MichalStrehovsky added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 17, 2018
@MichalStrehovsky MichalStrehovsky removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 21, 2018
@MichalStrehovsky MichalStrehovsky added this to the 3.0 milestone Oct 22, 2018
OBJECTREF * protectedObj, // this one can actually be NULL, consider using pMT is you don't need the object itself
PCODE * ppTarget)
PCODE * ppTarget,
BOOL throwOnConflict)
Copy link
Member

Choose a reason for hiding this comment

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

throwOnConflict [](start = 18, length = 15)

Is this supposed to be throwOnConflict or throwOnFailure? If throwOnFailure, there is also the type equivalence case to deal with.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I see that you handled the type equivalence case, which raises the question again on what this parameter actually means


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

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be throwOnConflict, like everywhere else. Fixed.

// We can resolve to exact method
pMD = this->GetMethodDescForInterfaceMethod(pInterfaceMT, pInterfaceMD);
pMD = this->GetMethodDescForInterfaceMethod(pInterfaceMT, pInterfaceMD, FALSE /* throwOnConflict */);
_ASSERTE(pMD != NULL);
Copy link
Member

Choose a reason for hiding this comment

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

Assertion no longer accurate

Copy link
Member

Choose a reason for hiding this comment

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

fIsExactMethodResolved should be false if pMT is NULL


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

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes. We would hit that assert now. Fixed.


DispatchSlot slot(pMT->FindDispatchSlot(token));
DispatchSlot slot(pMT->FindDispatchSlot(token, TRUE /* throwOnConflict */));

Copy link
Member

Choose a reason for hiding this comment

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

throwing here is going to do something odd in the debugger when stepping into a failing interface dispatch. I'm not convinced throwing is wrong, but I'm not sure what it'll do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, added that to the debugger work tracking issue (#15866).

Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

🕐

@MichalStrehovsky
Copy link
Member Author

Cc @sergiy-k

@MichalStrehovsky MichalStrehovsky merged commit 4b01ac0 into dotnet:master Nov 15, 2018
@MichalStrehovsky MichalStrehovsky deleted the fix16123 branch November 15, 2018 13:08
AaronRobinsonMSFT added a commit to AaronRobinsonMSFT/coreclr that referenced this pull request Feb 15, 2019
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants