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

Add libc::c_bool type. #128

Closed
wants to merge 1 commit into from
Closed

Conversation

jimblandy
Copy link
Contributor

This is a revised fix for issue #116. The original pull request was #125. From that request:

The libc crate is supposed to provide a Rust type corresponding to each C type that might appear in a public API, for use in making foreign calls. However, libc doesn't provide any type designated to correspond to a C bool.

In practice, Rust's bool and the bool of every C ABI I'm aware of are the same, so using the Rust bool directly in foreign function interfaces will work fine on these systems. But the libc crate's principle is to provide types that match by definition, which isn't true of Rust bool, so libc::c_bool is still valuable.

I considered whether it was better to let c_bool be u8 or bool. In both C and C++, converting a value to a bool forces it to be either zero or one. Using Rust bool for c_bool mimics this behavior, whereas u8 would allow Rust code to pass other values. I think forcing a C bool to a value other than zero or one is undefined behavior, but I can't find the language to that effect right now. Either way, it seems like Rust's bool is the better match.

And later:

I forgot to #include <stdbool.h>.

@jimblandy
Copy link
Contributor Author

Okay, so, changeset 31bd823 has no changes relative to master, and yet still fails the Travis CI checks. Filed issue #129.

@jimblandy
Copy link
Contributor Author

I've reset the add-bool branch to the original commit, so if you're not concerned with the Travis CI failures, this is ready to merge again.

This was referenced Jan 3, 2016
@brson brson added the relnotes label Jan 5, 2016
@brson
Copy link
Contributor

brson commented Jan 5, 2016

Why does the test build script need stdbool.h if the tests don't add any tests from it? Are there tests we can add - do the other primitives have tests?

@jimblandy
Copy link
Contributor Author

@brson asked:

Why does the test build script need stdbool.h if the tests don't add any tests from it?

In C11, bool isn't actually a type in the language; _Bool is. Only if you #include <stdbool.h> does bool get defined as a macro that expands to _Bool. (ISO/IEC 9899:201x 7.18)

The generated all.c file includes the following code:

            uint64_t __test_size_c_bool(void) { return sizeof(bool); }
            uint64_t __test_align_c_bool(void) { return __alignof__(bool); }


            uint32_t __test_signed_c_bool(void) {
                return (((bool) -1) < 0);
            }    

which are used by the size_align_c_bool and sign_c_bool functions in the generated all.rs.

So all.c won't even compile if we don't include <stdbool.h>.

Are there tests we can add - do the other primitives have tests?

There are no tests for c_schar or c_uchar, other than the ones analogous to the automatically generated tests I mentioned above.

@jimblandy
Copy link
Contributor Author

Actually, let's hold off on merging this for a bit, I had not pushed the version of the branch I thought I had.

@jimblandy jimblandy closed this Jan 6, 2016
@jimblandy
Copy link
Contributor Author

This (about to re-push) is the version I'd intended to push. This version:

  • uses Rust's bool as the analog to C's bool: both are restricted to be either 0 or 1, and both are a single byte on every known system.
  • Skips generating a signedness test for libc::c_bool, since Rust's bool has no signedness.

One concern about this choice is that, if some as-of-yet-unencountered C ABI uses something other than a single byte that is zero for false or one for true for its bool representation, it would require a breaking change to libc to accommodate that; everyone using libc::c_bool would have to stop storing true and false directly in libc::c_bool members, passing them as arguments, and so on.

I think this risk is slight, and outweighed by the benefit of being able to use Rust's bool directly on all the platforms we know of now.

@petrochenkov
Copy link
Contributor

There was an RFC proposing addition of c_bool, but it was rejected.
I'll repeat my questions from that RFC here:

  1. If c_bool is intended to be usable without explicit conversions, it has to be hardcoded as a bool alias. In this case why can't we use Rust bool in FFI without pseudonyms.
  2. Overview of representations of C _Bool and C++ bool in various ABIs would be nice. "bool is always 1 byte with values 0 or 1" sounds plausible, but I haven't seen this proved with references to ABI specs.

@jimblandy
Copy link
Contributor Author

@petrochenkov: Thanks for pointing me at the RFC; I didn't realize this had been discussed before.

It makes sense to keep discussion on that thread, rather than forking it here. So I'm going to close this pull request for now.

@jimblandy jimblandy closed this Jan 6, 2016
@jimblandy
Copy link
Contributor Author

@petrochenkov

Overview of representations of C _Bool and C++ bool in various ABIs would be nice. "bool is always 1 byte with values 0 or 1" sounds plausible, but I haven't seen this proved with references to ABI specs.

Okay, now you owe me one! :)

@alexcrichton
Copy link
Member

Thanks for the PR @jimblandy! I think @petrochenkov was gonna do what I was gonna do which was point in the way of the RFC issue :)

danielverkamp pushed a commit to danielverkamp/libc that referenced this pull request Apr 28, 2020
Just needed some `constify_imm8!` treatment

Closes rust-lang#59
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 this pull request may close these issues.

4 participants