search

rust-lang / libc

Raw bindings to platform APIs for Rust
https://docs.rs/libc
Apache License 2.0
2.08k stars 1.04k forks source link

Wrong size types for `struct flock` constants #1619

Open quasicomputational opened 4 years ago

quasicomputational commented 4 years ago

On x8664 Linux (at least; I haven't looked at all at other platforms), `F(RD|WR|UN)LCKandSEEK_(SET|CUR|END)are all defined asc_int. However, theflockstruct usesc_shortfor the fields that they go in. [man 2 fcntl](http://man7.org/linux/man-pages/man2/fcntl.2.html), section 'Advisory record locking', and [POSIX](https://pubs.opengroup.org/onlinepubs/009695399/basedefs/fcntl.h.html) confirm that they should beshort, notint`.

gnzlbg commented 4 years ago

PRs welcome, I can mentor if required.

quasicomputational commented 4 years ago

A nit that's just occurred to me: SEEK_* are also used for lseek(2), where they do need to be an int. This is possible in C because they're defined as macros, but some other solution will be needed for Rust (e.g., separate constants with different types).

gnzlbg commented 4 years ago

If they are defined in C as macros, then their type is polymorphic (the value in the macro can coerce to any type for which the coercion is correct), and there is no way to express that in Rust yet, so that c_int is pretty much as good as anything else.

Typically, we try to choose the type along two axes: consistency (most of these macros use c_int already), and ergonomics (what's the type that most APIs in which these macros are used expect).

If there are multiple APIs in which these macros are used, and they use different types, then currently the ergonomics of some code are going to suffer.

If we ever get polymorphic constants, we can revisit this issue, and change the type so that it works for all C APIs that use these values. That would be a net win.

Right now, and in this particular case, changing the type is also a major breaking change, and I don't think it is worth it.

What do you think ?