-
Notifications
You must be signed in to change notification settings - Fork 13.3k
simplify processing of ConstVal objects when not all variants are legal #26935
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
Why not add another |
I'd love to have a repr C discriminant enum for every enum and an easy way to convert to it... (oh + subset-enums xD ) but since we don't have that, I'll just hand-code them |
hmm actually there is a reason... another enum will require touching all those places again and only save us from modifying the hand-flattened |
ConstVal::Binary(_) => "binary array", | ||
ConstVal::Struct(..) => "struct", | ||
ConstVal::Tuple(_) => "tuple" | ||
const_val => const_val.description(), |
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.
You might want to keep around the special-case for ConstVal::Int? The fact that the integer is negative is important.
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'll just change the description method to emit that information, too
a41225f
to
441b994
Compare
updated to pass around the actual |
ping @eddyb |
@bors r+ |
📌 Commit 441b994 has been approved by |
r? @eddyb Adding new variants is annoying as one needs to modify all these places that **don't** handle the new variant. I chose not to use `Display` as I don't think it is appropriate.
r? @eddyb
Adding new variants is annoying as one needs to modify all these places that don't handle the new variant.
I chose not to use
Display
as I don't think it is appropriate.