-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Disallow TypedReference and byref types in MakeArrayType #55403
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
Conversation
|
Tagging subscribers to this area: |
|
Tagging subscribers to this area: Issue DetailsFixes #39001
|
fanyang-mono
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| check_for_invalid_array_type (klass, error); | ||
| check_for_invalid_array_type (type, error); | ||
| return_val_if_nok (error, MONO_HANDLE_CAST (MonoReflectionType, NULL_HANDLE)); | ||
| MonoClass *klass = mono_class_from_mono_type_internal (type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it is better to pass MonoClass *klass to check_for_invalid_array_type, since line 6298 will always be executed. That way we could save one function call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the problem is that if you start with a MonoType and then call mono_class_from_mono_type_internal it is not an exact roundtrip. In particular you lose the MonoType:byref bit. Because a MonoClass is not able to represent a byref type. (This is kind of an annoying limitation in Mono but fixing it is probably a ton of work).
I thought about instead returning the klass from check_for_invalid_array_type, but it's kind of a weird incidental return value. In any case mono_class_from_mono_type_internal is pretty quick to call since it's just a switch on a value (MonoType:type) that we already looked at.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha! My original thought was to returning the klass from check_for_invalid_array_type. But since it is not costly, I am okay with current implementation.
|
Hello @lambdageek! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Fixes #39001