Skip to content

Make {Try}GetAlternateLookup methods for Dictionary/HashSet instance instead of extensions #106107

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

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

stephentoub
Copy link
Member

We made them extension methods instead of instance methods to avoid potential native code size bloat. But the ergonomics of using these without partial generic inference is a bit painful, and we've had reports that it makes them harder to understand. This moves them to be instance methods, and we'll measure the impact on code size to re-evaluate the decision.

@stephentoub stephentoub added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 8, 2024
@stephentoub stephentoub added this to the 9.0.0 milestone Aug 8, 2024
@ghost
Copy link

ghost commented Aug 8, 2024

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
@ghost
Copy link

ghost commented Aug 8, 2024

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

…hSet

We made them extension methods instead of instance methods to avoid potential native code size bloat. But the ergonomics of using these without partial generic inference is a bit painful, and we've had reports that it makes them harder to understand. This moves them to be instance methods, and we'll measure the impact on code size to re-evaluate the decision.
@stephentoub stephentoub removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 8, 2024
@stephentoub stephentoub changed the title Try making {Try}GetAlternateLookup instance methods on Dictionary/HashSet Make {Try}GetAlternateLookup instance methods on Dictionary/HashSet instead of extensions Aug 8, 2024
@stephentoub stephentoub changed the title Make {Try}GetAlternateLookup instance methods on Dictionary/HashSet instead of extensions Make {Try}GetAlternateLookup methods for Dictionary/HashSet instance instead of extensions Aug 8, 2024
@stephentoub
Copy link
Member Author

stephentoub commented Aug 8, 2024

Discussed offline with @jkotas, @MichalStrehovsky, @terrajobst, and @eiriktsarpalis. After gathering some data and discussing, it turns out that generic methods (whether or not on a generic type) don't have the same concerning effect on Native AOT code size as do non-generic methods on generic types. We've agreed to go ahead with this improvement.

@stephentoub stephentoub merged commit 44b6b2a into dotnet:main Aug 8, 2024
144 of 147 checks passed
@stephentoub stephentoub deleted the movegetalternate branch August 8, 2024 15:18
@github-actions github-actions bot locked and limited conversation to collaborators Sep 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants