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.

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

Revert "Revert "Re-add static interface trimming with more testing"" #2859

Closed
wants to merge 1 commit into from

Conversation

jtschuster
Copy link
Member

@jtschuster jtschuster commented Jun 22, 2022

The failures that led to reverting static interface trimming (https://github.com/dotnet/runtime/runs/6900652466 - System.Security.Cryptography.CngKey's IDisposable interface implementation was trimmed) seem to be related to the build using the wrong version of the linker (#2848). When using the linker directly with these changes, I haven't been able to repro the issue. This PR reverts the revert and adds the static interface trimming capability back.

@jtschuster jtschuster marked this pull request as ready for review June 24, 2022 16:05
@jtschuster jtschuster requested a review from marek-safar as a code owner June 24, 2022 16:05
Copy link
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

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

This is just a revert of 4bed0da, right? Please force push so that the commits in this PR are just a single revert commit, with any extra changes in separate commits on top of that. This will make it easier to review just the changes if there are any.

@jtschuster jtschuster force-pushed the rereaddstaticinterfaces branch from 0448fb7 to 3a618bb Compare June 24, 2022 20:45
Copy link
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

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

Thanks!

@jtschuster
Copy link
Member Author

Now that static virtual interface methods are possible, more tests need to be written before this gets reviewed and merged.

@jtschuster
Copy link
Member Author

#2868 encompasses these changes

@jtschuster jtschuster closed this Jul 13, 2022
jtschuster added a commit that referenced this pull request Aug 3, 2022
…ace methods (#2868)

Fixes #2865
Also addresses marking of all static interface methods encompassing the changes from #2859, and updates the way that all interface methods are marked. Whether or not we mark an interface method due to its base method is now separated from marking other virtual methods and the marking is postponed to ProcessMarkedTypesWithInterface. In ProcessMarkedTypesWithInterfaces, interface implementations are marked, and methods that implement a marked/implemented interface are marked. Tests for static interface methods have also been updated.

Co-authored-by: Sven Boemer <sbomer@gmail.com>
agocke pushed a commit to dotnet/runtime that referenced this pull request Nov 16, 2022
…ace methods (dotnet/linker#2868)

Fixes dotnet/linker#2865
Also addresses marking of all static interface methods encompassing the changes from dotnet/linker#2859, and updates the way that all interface methods are marked. Whether or not we mark an interface method due to its base method is now separated from marking other virtual methods and the marking is postponed to ProcessMarkedTypesWithInterface. In ProcessMarkedTypesWithInterfaces, interface implementations are marked, and methods that implement a marked/implemented interface are marked. Tests for static interface methods have also been updated.

Co-authored-by: Sven Boemer <sbomer@gmail.com>

Commit migrated from dotnet/linker@118bdca
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants