Skip to content

Collect all MethodDesc fields in one place #101565

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 26, 2024
Merged

Collect all MethodDesc fields in one place #101565

merged 3 commits into from
Apr 26, 2024

Conversation

@AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Apr 25, 2024

Clarify the purpose of a field by renaming.

@AaronRobinsonMSFT
Collect all MethodDesc fields in one place
2c2edf2 
Clarify the purpose of a field by renaming.
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 9.0.0 milestone Apr 25, 2024
@ghost ghost added the area-VM-coreclr label Apr 25, 2024
@dotnet-policy-service
Copy link
Contributor

dotnet-policy-service bot commented Apr 25, 2024

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

@davidwrighton
Copy link
Member

davidwrighton commented Apr 25, 2024

Why are you renaming the flags field to the classification field? It still has things that are not part of the classification of the MethodDesc.

@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented Apr 25, 2024

Why are you renaming the flags field to the classification field? It still has things that are not part of the classification of the MethodDesc.

All values seem to come from the MethodDescClassification enum. Did I miss one that isn't from the enum?

@jkotas
Copy link
Member

jkotas commented Apr 25, 2024

All values seem to come from the MethodDescClassification enum. Did I miss one that isn't from the enum?

The enum is poorly named. It has classification and a bunch of other stuff.

@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented Apr 25, 2024

All values seem to come from the MethodDescClassification enum. Did I miss one that isn't from the enum?

The enum is poorly named. It has classification and a bunch of other stuff.

Yep. @davidwrighton and I spoke offline. I'm going to rename the enum and change the field name back.

davidwrighton
davidwrighton approved these changes Apr 25, 2024
View reviewed changes
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.

This looks good.

@build-analysis build-analysis bot mentioned this pull request Apr 26, 2024
Closed
@AaronRobinsonMSFT AaronRobinsonMSFT merged commit 7a08e67 into dotnet:main Apr 26, 2024
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the methoddesc_update branch April 26, 2024 01:00
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
@AaronRobinsonMSFT @matouskozak
Collect all MethodDesc fields in one place (dotnet#101565)
18844cf 
* Collect all MethodDesc fields in one place

* Change enum name.

* Remove irrelevant macro - METHOD_MAX_RVA.
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
@AaronRobinsonMSFT @michaelgsharp
Collect all MethodDesc fields in one place (dotnet#101565)
668ee33 
* Collect all MethodDesc fields in one place

* Change enum name.

* Remove irrelevant macro - METHOD_MAX_RVA.
@github-actions github-actions bot locked and limited conversation to collaborators May 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@davidwrighton davidwrighton davidwrighton approved these changes

Assignees

@AaronRobinsonMSFT AaronRobinsonMSFT

Labels

area-VM-coreclr

Projects

None yet

Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@AaronRobinsonMSFT @davidwrighton @jkotas