-
Notifications
You must be signed in to change notification settings - Fork 253
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
c2rust transpile
: When casting bool
s to floats, go through the integral type u8
#1030
Conversation
It wasn't clear to me whether this should be implemented as a new CastKind (BooleanToFloating) or treated as a special case of IntegralToFloating. |
Hi @kkysen, pinging you because I see that you are a major contributor to this project. I apologize if you aren't the right person to talk to. What's the process for getting this reviewed? Thanks in advance! |
Sorry for continuing to tag people, I just want to try one more time. @spernsteiner are you the right person to talk to? Please let me know if I should just abandon this. |
Hi @dgherzka! Sorry for taking a while to review this; I saw your ping but forgot to take a look. I'll review it now, and your other PRs and issues. |
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.
Thanks for this PR! Sorry we took so long to review it, but we really appreciate it; thanks for fixing this bug!
It wasn't clear to me whether this should be implemented as a new CastKind (BooleanToFloating) or treated as a special case of IntegralToFloating.
bool
technically is an integral type, so I think a case of IntegralToFloating
should be fine, and the new code handling this case fits in nicely.
And the simple test case looks good and is enough to catch this bug, so everything LGTM. Thanks!
c2rust transpile
: When casting bool to float, go through integral type
c2rust transpile
: When casting bool to float, go through integral typec2rust transpile
: When casting bool
to float
, go through integral 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.
As for casting through u8
to go bool
=> u8
=> floating type, I'm not sure if casting through c_int
would be more correct in terms of what C is supposed to do, but casting through u8
should always be correct, and it seems more idiomatic for Rust, so I think that's fine. @thedataking, do you think that's fine? Are there any subtleties in choosing the intermediary integral type?
c2rust transpile
: When casting bool
to float
, go through integral typec2rust transpile
: When casting bool
s to floats, go through the integral type u8
Similar to immunant#1030. Fixes immunant#1077.
Previously, C code that cast
bool
s to floating types, like thiswould try to do so directly in Rust, like this
which isn't allowed, resulting in errors like this
This fixes things by emitting this Rust instead by casting through the integral type
u8
: