-
-
Notifications
You must be signed in to change notification settings - Fork 638
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
Simplify MetadataBuilder.GetConstantType. #702
base: master
Are you sure you want to change the base?
Conversation
The value of the "Type" column in the constant table is now determined from the object's type; not the field or parameter's declared type.
Thanks for the PR. When Cecil reads an enum constant, it's not going to instantiate an enum, meaning the boxed value will be of the enum's underlying type. So something might be off in the case where the field's type is an enum. |
This is not a problem at all. Try running the following code: using System;
public static class C {
public static void Main() {
object x = 3;
Console.WriteLine((DayOfWeek) x);
}
} It prints Besides, your concerns are towards Cecil's constant reader, while this PR eliminates the use of the assembly resolver from the constant writer. The type of a constant is declared twice. Once, at the field's declaration as a type metadata token, and once again at the Mismatches between the two constant types can happen and it's the developer's responsibility to avoid them. Otherwise we have cases that dnSpy displays as |
Lol, thanks for the C# lesson I didn't ask for. What I'm saying above is that this is changing how a piece of metadata read from an assembly is going to be written back. enum Letter { A, B }
class Foo {
public const Letter FirstLetter = L.A;
} If the constant record is serialized as an enum in the original assembly, writing it with Cecil will write a constant record using the underlying type of the enum. It doesn't mean that it doesn't work. Cecil doesn't do a perfect job at writing back what it read, but it's trying. It also doesn't mean that it's not an acceptable trade-off. |
I am glad that the PR is under consideration but I still can't understand the explanation of your trade-off.
The values of a constant record are serialized as primitive values, not enums. This is the generated IL of your above snippet: The
Again, I am glad that the PR is under consideration. :-) |
The value of the "Type" column in the constant table is now determined from the object's type; not the field or parameter's declared type, avoiding calling the resolver when the constant is an enum.
Type.GetTypeCode()
has been verified to return the underlying type of enum types:Type.GetTypeCode(typeof(DayOfWeek))
returnsTypeCode.Int32
for example.Fixes #886.