Skip to content

"as bool" does not properly convert its argument. #7311

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

Closed
thomcc opened this issue Jun 22, 2013 · 3 comments
Closed

"as bool" does not properly convert its argument. #7311

thomcc opened this issue Jun 22, 2013 · 3 comments

Comments

@thomcc
Copy link
Member

thomcc commented Jun 22, 2013

foo as bool should result in an i1, not an i8. This can lead to various unexpected or hard to debug behavior. See:

rusti> 3 as bool as u8
3
rusti> (3 as bool)
true
rusti> (2 as bool)
true
rusti> std::bool::xor(true, true)
false
rusti> std::bool::xor((3 as bool), (2 as bool))
true
rusti> 0xffff as bool as u8
255
rusti> 0xff00 as bool
false
@thestinger
Copy link
Contributor

I think we're handling bool very wrong right now. It should be stored as u8 on the stack, but passed as i1 zeroext in parameters like clang. It should be turned into an i1 immediately after a load and extended to a u8 for a store.

@thestinger
Copy link
Contributor

I don't think we should allow as conversions on bool at all because you can use == 0. Re-nominating for the backwards compatible milestone.

@metajack
Copy link
Contributor

metajack commented Sep 5, 2013

Visiting for triage. Looks like this will be fixed shortly. Nothing to add.

bors added a commit that referenced this issue Sep 5, 2013
This is currently unsound since `bool` is represented as `i8`. It will
become sound when `bool` is stored as `i8` but always used as `i1`.

However, the current behaviour will always be identical to `x & 1 != 0`,
so there's no need for it. It's also surprising, since `x != 0` is the
expected behaviour.

Closes #7311

d0a1176 r=huonw
e4a76e6 r=thestinger
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants