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

[cDAC] Set RejitFlags to use a known underlying type #109935

Merged
merged 2 commits into from
Nov 19, 2024

Conversation

max-charlamb
Copy link
Contributor

@max-charlamb max-charlamb commented Nov 18, 2024

The previous implementation using an enum should be the size of a uint32_t but it isn't explicitly defined. To make the size of RejitFlags more clear, changing it to an enum class with an explicit uint32_t backing.

Will be used #109936 by the cDAC GetMethodDescData to fetch ReJIT information.

Copy link
Contributor

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

Copy link
Member

@davmason davmason left a comment

Choose a reason for hiding this comment

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

LGTM modulo the comment about usage.

@@ -184,7 +184,7 @@ class ILCodeVersion
HRESULT SetActiveNativeCodeVersion(NativeCodeVersion activeNativeCodeVersion);
#endif //DACCESS_COMPILE

enum RejitFlags
enum class RejitFlags : uint32_t
Copy link
Member

Choose a reason for hiding this comment

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

Could we do some C++ to make us not have to write out ILCodeVersion::RejitFlags:: every time? I think we could put using enum RejitFlags in the ILCodeVersion class definition so the usage is the same. It feels too long to type out all of it every time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is possible to do for enum class's in C++14. It looks like using enum is only available in C++20 and higher.

I see some approaches online manually dumping each enum value, but that feels worse than referencing them directly. https://stackoverflow.com/a/48932002

Copy link
Member

Choose a reason for hiding this comment

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

It is a bit unfortunate. Sometimes in the past I've simply elevated the enum to no longer be contained within the class when I do this conversion. Then I can just use RejitFlags::BLAH wherever it needs to be used. You still have to touch all of the places where the enum is touched, but the major reason to put an enum within ILCodeVersion was probably to give its members a namespace to live in, and enum class does that all by itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pulled the enum out of ILCodeVersion. Now it is referenced with RejitState::<enum_value> instead of ILCodeVersion::<enum_value>.

@max-charlamb max-charlamb marked this pull request as ready for review November 19, 2024 16:43
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.

I've made another comment about how I might have pulled the enum out of the ILCodeVersion class to avoid the ILCodeVersion::RejitState prefix on everything, but it is not critical to me.

@max-charlamb max-charlamb merged commit 161f346 into dotnet:main Nov 19, 2024
90 checks passed
mikelle-rogers pushed a commit to mikelle-rogers/runtime that referenced this pull request Dec 10, 2024
set RejitFlags enum to use uint32_t as backing type for use in cDAC
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants