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

Remove virtual modifier from Calendar.CurrentEraValue #104453

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Jul 4, 2024

This change enables inlining of the CurrentEraValue property.

MihuBot/runtime-utils#475 (comment)

@xtqqczze xtqqczze changed the title Remove virtual modifer from Calendar.CurrentEraValue Remove virtual modifier from Calendar.CurrentEraValue Jul 4, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 4, 2024
Copy link
Contributor

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

@tarekgh tarekgh added this to the 9.0.0 milestone Jul 4, 2024
@stephentoub
Copy link
Member

This change enables inlining of the CurrentEraValue property.

The change is fine, but with regards to this explanation, there are zero overrides of this virtual method such that this base will always be the target of the invocation. As such, I'd expect dynamic PGO to optimize this well, such that CurrentEraValue would still be inlinable, just with an extra type check in front of it.

@stephentoub stephentoub merged commit 85eb0b5 into dotnet:main Jul 5, 2024
142 of 146 checks passed
@xtqqczze xtqqczze deleted the CurrentEraValue branch July 5, 2024 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Globalization community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants