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

Document the size of bool #46156

Merged
merged 1 commit into from
Feb 3, 2018
Merged

Document the size of bool #46156

merged 1 commit into from
Feb 3, 2018

Conversation

SimonSapin
Copy link
Contributor

No description provided.

@SimonSapin
Copy link
Contributor Author

It is always 1, right?

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @BurntSushi (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 21, 2017
@steveklabnik
Copy link
Member

steveklabnik commented Nov 21, 2017

This is historically not guaranteed on purpose rust-lang/rfcs#954

We discussed this at triage today, and the general feeling was that we probably don't want to commit to the representation of Rust's bool type. We may want to apply future optimizations to the representation or semantics, and this sort of restriction may prevent us from doing so.

/cc @rust-lang/lang

See also rust-lang/rfcs#992

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Nov 21, 2017

Worth noting that rust-lang/rfcs#954 proposed far more than documenting the size, it proposed guaranteeing a encoding of true and false as bytes and furthermore ties it to the C ABI (it's legal for the C compiler to make _Bool larger than one byte, although in practice that doesn't seem to be common).

In contrast, guaranteeing a size_of of 1 seems entirely harmless, because what else could it sensibly be? (Edit: See correction below) Even if future Rust compiler could sometimes packs bools in unused bits in aggregates, a size_of of 0 would break things such as Box::new(true). A larger size_of would be legal, but would not be an "optimization".

@durka
Copy link
Contributor

durka commented Nov 21, 2017

The values true=1, false=0 are documented: https://doc.rust-lang.org/reference/behavior-considered-undefined.html

@bluss
Copy link
Member

bluss commented Nov 21, 2017

The reference is not official rust docs and it is not a real reference yet, unfortunately.

@est31
Copy link
Member

est31 commented Nov 21, 2017

@bluss then remove that statement from the reference. And no it is official documentation. Otherwise why is it on the doc.rust-lang.org domain?

@bluss
Copy link
Member

bluss commented Nov 21, 2017

Nightly version has a better banner and a link to the nursery repo where it's developed: https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html

@durka
Copy link
Contributor

durka commented Nov 21, 2017

Then we should close this PR as writing down the size of bool won't have any weight... is there a list of "decisions that have actually been made and can be trusted"?

@cramertj
Copy link
Member

@durka IMO the authoritative Rust documentation is the standard library's documentation, as the guarantees it specifies are part of the library, and therefore part of Rust's stability guarantees.

@scottmcm
Copy link
Member

Hmm, so the impact of this change would be to put the compile-time-validity of transmute::<bool, u8> inside the stability guarantee? I understand writing code that relies on size_of::<u8>() == 1; is it particularly valuable to be able to rely on size_of::<bool>() == 1 in unsafe?

@est31
Copy link
Member

est31 commented Nov 21, 2017

So as the ABI of bool seems to be not specified, please see my PR #46176 to lint about bool in FFI usage.

@egilburg
Copy link

Perhaps document i128 and u128 too?

@hanna-kruppe
Copy link
Contributor

It appears that C doesn't specify the size of _Bool (it just has to be able to store 0 and 1), so if we want to specify bool as being compatible with _Bool that would theoretically clash with documenting the size to be 1. I say "theoretically" because I'm not aware of a platform that makes _Bool larger (other than possibly architectures where char isn't 8 bit, but those aren't really supported by Rust anyway).

est31 added a commit to est31/rust that referenced this pull request Nov 22, 2017
The ABI of bool is reserved, see these links:

* rust-lang/rfcs#954 (comment)
* rust-lang#46156 (comment)
* rust-lang/rfcs#992

Currently the improper_ctypes lint is inconsistent
with that position by treating bools as if they had
a specified ABI.

To fix this inconsistency, this changes treatment of
bools by the lint.

This might break some code, so possibly it has to
be phased in slowly...
@withoutboats
Copy link
Contributor

IMO the size of bool should be defined as the same as the size of _Bool on that platform, which we should helpfully document as being one byte essentially always.

@withoutboats
Copy link
Contributor

Basically I think we should just accept RFC 954 at this point.

@Havvy
Copy link
Contributor

Havvy commented Nov 22, 2017

I added the table in #44648 and checked what people said on IRC around the time. I'm not seeing any reference to bool in either location. Looks like I completely missed it on oversight.

I do have a reference PR where I added it though, but it looks like I'll have to remove that line. 😣

About the UB section of the reference — it's immaterial to this discussion. It's stating that the valid values of bool are 0 and 1. There's nothing about the size of the type. Or are we even saying that we can have bools where false is not 0 and true is not 1?

@vojtechkral
Copy link
Contributor

vojtechkral commented Nov 22, 2017

IMO the size of bool should be defined as the same as the size of _Bool on that platform

I wonder why should bool be treated differently than integer types. With integer types, Rust defines its types such that they are as sane and consistent across platforms as possible (ie. i32, usize etc.) and relies on libc to provide C FFI interop types, which seems to work reasonably well. Why isn't the same strategy applied to bool? That is, keep it sane inside Rust (ie. sized 1) and provide a libc bool type for C FFI.

@withoutboats
Copy link
Contributor

withoutboats commented Nov 22, 2017

That also seems fine.

I think the larger thrust of my objective is that we should stop delaying action based on theoretical concerns. There are neither viable optimizations nor major platforms which conflict with specifying the obvious representation of bool. Any solution in which we specify the repr of boolean as what it has been and will always be on major platforms is good.

@SimonSapin
Copy link
Contributor Author

hsivonen wrote in a thread that mentioned this PR:

I'm so glad Rust committed to two's complement despite it ruling out FFI interop with some C implementations. I wish today's Rust had more of the “UTF-8 everywhere, two's complement everywhere” attitude.

I think that’s a good point, and we should decide that Rust’s bool always has the obvious representation. If they turn out to be real, FFI concerns can be addressed with conversions in the libc crate.

@kennytm kennytm added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Nov 24, 2017
@aturon
Copy link
Member

aturon commented Nov 28, 2017

Nominated for lang team discussion.

@kennytm kennytm added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Dec 4, 2017
@SimonSapin
Copy link
Contributor Author

As an aside, it would be pretty nice to have u1 arrays and slices compile to space-efficient bitmaps!

You’re looking for https://crates.io/crates/bit-vec. This is not possible with [T; N] / [T] / Vec<T> because in these each element needs to have a distinct address because indexing returns &T or &mut T.

@scottmcm
Copy link
Member

As an aside, it would be pretty nice to have u1 arrays and slices compile to space-efficient bitmaps!

C++ has experience with that, and it turned out to be better as opt-in instead of magic: https://isocpp.org/blog/2012/11/on-vectorbool

@kennytm
Copy link
Member

kennytm commented Jan 10, 2018

Triage ping @rust-lang/lang! It seems we still haven't got a clear decision...

@nikomatsakis
Copy link
Contributor

Cross-posting from the internals thread:

We finally got around to discussing this in the @rust-lang/lang meeting. We had the following questions, perhaps someone may be in a position to answer authoritatively:

  • Are there any extant platforms where _Bool in the C ABI is permitted to take values other than 0 or 1?
    • How I understood prior answers is "no, truth-y values will coerce to 1"
  • Does the C standard permit platforms where _Bool is permitted to take values other than 0 or 1?
  • Side question: are there extant platforms where sizeof(_Bool) != 1 ?
    • We believe the C standard permits other sizes, but weren't clear if anyone took advantage of that freedom.

Our preference is to define bool as _Bool. This would be what most people expect and avoid massive breakage. If the C standard requires values of 0 or 1, then there seems to be basically no drawback. Otherwise, it implies a potential performance pitfall on matches (matches on bools, mind you) but doesn't seem like a huge problem. Still, it'd be good to know the answers to those questions before reaching a final consensus.

Relevant to this RFC, @SimonSapin or others, do you think that defining bool to be 1 byte everywhere is better than _Bool? If so, it'd be good to discuss the case for that further. (I confess to not having read every comment here, so I'd appreciate pointers into the discussion.)

@petrochenkov
Copy link
Contributor

Additional question for the list:

  • Are there any extant platforms where C++ bool and C _Bool have different ABIs?

C _Bool is a late addition to the language that is largely unused in C interfaces, so C++ bool should be much more important from practical FFI point of view.

@vojtechkral
Copy link
Contributor

@nikomatsakis

Does the C standard permit platforms where _Bool is permitted to take values other than 0 or 1?

From N1570, §6.2.5:

An object declared as type _Bool is large enough to store the values 0 and 1.

§6.3.1.2:

When any scalar value is converted to _Bool, the result is 0 if the value compares equal
to 0; otherwise, the result is 1. 59)

Ie. I believe from the standard's point of view _Bool can only have values 1 and 0.

cppreference.com also notes a consequence:

Note that conversion to _Bool does not work the same as conversion to other integer types: (bool)0.5 evaluates to 1, whereas (int)0.5 evaluates to ​0​.

@SimonSapin
Copy link
Contributor Author

@SimonSapin or others, do you think that defining bool to be 1 byte everywhere is better than _Bool?

(Replying since I’m mentioned by name. “Better” is a big word. I have no idea. I submitted this PR because it had not occurred to be that this could be otherwise.)

@est31
Copy link
Member

est31 commented Feb 2, 2018

The FCP has been resolved: #46176 (comment)

Please merge this now!

@withoutboats
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 2, 2018

📌 Commit 219ba51 has been approved by withoutboats

@kennytm kennytm 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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Feb 2, 2018
@kennytm
Copy link
Member

kennytm commented Feb 2, 2018

@bors rollup

kennytm added a commit to kennytm/rust that referenced this pull request Feb 2, 2018
bors added a commit that referenced this pull request Feb 3, 2018
Rollup of 10 pull requests

- Successful merges: #46156, #47829, #47842, #47898, #47914, #47916, #47919, #47942, #47951, #47973
- Failed merges: #47753
@bors bors merged commit 219ba51 into rust-lang:master Feb 3, 2018
@SimonSapin SimonSapin deleted the patch-14 branch March 30, 2018 13:52
@nagisa nagisa mentioned this pull request Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.