-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
Support primitives in type info reflection #151123
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
base: main
Are you sure you want to change the base?
Conversation
|
|
7fccb29 to
1066081
Compare
1066081 to
8281828
Compare
This comment has been minimized.
This comment has been minimized.
8281828 to
8d00b3b
Compare
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 also added a ty: TypeId to all primitive variants to represent their own type. The rationale is that users won't need to manually retrieve the TypeId when accessing type info (especially through Type::of::<T>() usage). I'm not sure if this approach is advisable, and whether it's necessary to also provide a ty: TypeId for other variants like Tuple and Array.
| /// The type id of signed integer type. | ||
| pub ty: TypeId, | ||
| /// The bit width of the signed integer type. | ||
| pub bit_width: usize, |
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'm not sure whether we should use usize or u64 here to represent the bit width.
Besides, as Option is not used here, reflection on isize and usize will fall into the Int and Uint variants, and users cannot distinguish between them, which may be an issue.
Is it necessary for end users to distinguish between {u,i}size and {u,i}int? If yes, should we add a separate field or add distinct variants to indicate it?
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 think having a single Int variant that has a signed: bool field is nicer. It avoids some recurring matches that do Int(_) | Uint(_)
This comment has been minimized.
This comment has been minimized.
8d00b3b to
80ae7d8
Compare
Support {bool,char,int,uint,float,str} primitive types for feature
`type_info` reflection.
80ae7d8 to
79ec275
Compare
In that case, it might be better to place |
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.
Yea I think we can leave off the extra TypeId field. I understand that reflection libraries have that on their Opaque variant, but they can still do that when processing the rustc reflection info. I'd rather users invoke TypeId::of manually when they also need the type id
| /// The type id of signed integer type. | ||
| pub ty: TypeId, | ||
| /// The bit width of the signed integer type. | ||
| pub bit_width: usize, |
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 think having a single Int variant that has a signed: bool field is nicer. It avoids some recurring matches that do Int(_) | Uint(_)
| macro_rules! dump_types { | ||
| ($($ty:ty),+ $(,)?) => { | ||
| $(println!("{:#?}", const { Type::of::<$ty>() });)+ | ||
| }; | ||
| } |
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.
ohh great idea, thanks!
Tracking issue: #146922
#![feature(type_info)]This PR supports {
bool,char,int,uint,float,str} primitive types for featuretype_inforeflection.r? @oli-obk