Skip to content
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

Consider Scalar to be a bool only if its unsigned #80562

Merged
merged 1 commit into from
Jan 30, 2021

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Dec 31, 2020

This seems right, given that conceptually bools are unsigned, but the
implications of this change may have more action at distance that I'm
not sure how to exhaustively consider.

For instance there are a number of cases where code attaches range
metadata if scalar.is_bool() holds. Supposedly it would no longer be
attached to the repr(i8) enums? Though I'm not sure why booleans are
being special-cased here in the first place...

Fixes #80556

cc @eddyb

@rust-highfive
Copy link
Collaborator

r? @varkor

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 31, 2020
@nagisa
Copy link
Member Author

nagisa commented Dec 31, 2020

An alternative fix could be to set sext here if the scalar is signed here.

@varkor
Copy link
Member

varkor commented Dec 31, 2020

r? @eddyb, who is more familiar with the subtleties here. There already seems to be special casing regarding boolean values for signed integers, e.g.

let signed = match tag_scalar.value {
// We use `i1` for bytes that are always `0` or `1`,
// e.g., `#[repr(i8)] enum E { A, B }`, but we can't
// let LLVM interpret the `i1` as signed, because
// then `i1 1` (i.e., `E::B`) is effectively `i8 -1`.
Int(_, signed) => !tag_scalar.is_bool() && signed,
_ => false,
};

which would presumably become vacuous if signed integers are never treated as booleans.

@rust-highfive rust-highfive assigned eddyb and unassigned varkor Dec 31, 2020
@jyn514 jyn514 added A-codegen Area: Code generation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 4, 2021
@camelid camelid added the A-layout Area: Memory layout of types label Jan 22, 2021
@eddyb
Copy link
Member

eddyb commented Jan 29, 2021

Sorry for the delay, r=me with @camelid's suggestion applied.

This seems right, given that conceptually bools are unsigned, but the
implications of this change may have more action at distance that I'm
not sure how to exhaustively consider.

For instance there are a number of cases where code attaches range
metadata if `scalar.is_bool()` holds. Supposedly it would no longer be
attached to the `repr(i8)` enums? Though I'm not sure why booleans are
being special-cased here in the first place...

Fixes rust-lang#80556
@nagisa
Copy link
Member Author

nagisa commented Jan 29, 2021

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Jan 29, 2021

📌 Commit 915a04e has been approved by eddyb

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 29, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 30, 2021
Rollup of 16 pull requests

Successful merges:

 - rust-lang#79023 (Add `core::stream::Stream`)
 - rust-lang#80562 (Consider Scalar to be a bool only if its unsigned)
 - rust-lang#80886 (Stabilize raw ref macros)
 - rust-lang#80959 (Stabilize `unsigned_abs`)
 - rust-lang#81291 (Support FRU pattern with `[feature(capture_disjoint_fields)]`)
 - rust-lang#81409 (Slight simplification of chars().count())
 - rust-lang#81468 (cfg(version): treat nightlies as complete)
 - rust-lang#81473 (Warn write-only fields)
 - rust-lang#81495 (rustdoc: Remove unnecessary optional)
 - rust-lang#81499 (Updated Vec::splice documentation)
 - rust-lang#81501 (update rustfmt to v1.4.34)
 - rust-lang#81505 (`fn cold_path` doesn't need to be pub)
 - rust-lang#81512 (Add missing variants in match binding)
 - rust-lang#81515 (Fix typo in pat.rs)
 - rust-lang#81519 (Don't print error output from rustup when detecting default build triple)
 - rust-lang#81520 (Don't clone LLVM submodule when download-ci-llvm is set)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a5c12ea into rust-lang:master Jan 30, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-layout Area: Memory layout of types S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE when returning an repr(i8) enum from an extern c function
8 participants