Fix usage of isize / usize

[dcampbell24]

I am not totally sure about whether it makes sense to be casting between C and Rust integer types, but currently isize is being used in several places to map to a c_int, which is wrong. c_int's can be as small as 16 bits and are not related to pointer size, so, as is, values can silently overflow on the Rust side.

[ketsuban]

Are any of the "several places" not listed in this commit? @eagleflo did some heroic work removing uses of isize/usize as part of #273, but @jpernst just replaced int/uint literals with isize/usize ones which isn't necessarily the correct thing to do. (In fairness, @jpernst was only resolving warnings, and nobody's reported having problems which turned out to be due to integer overflow.)

[jpernst]

Yeah, my commit was solely concerned with removing a deprecated feature. @eagleflo did a great job actually cleaning up the API to use more appropriate types. Taking a closer look, @davekong is right that there are a few cases of this still lingering. The main one I see so far is Window::new which takes the window size as isize, while the c header uses "int" which will almost certainly be a different size.

[dcampbell24]

Grepping for "as c_int" in all the files is how I came across the examples I am talking about.

[eagleflo]

I only cleaned up the event side. If you grep/ack/ag for isize or usize in rust-sdl2 codebase you will find a lot of other places where isize & usize are still used.

I'm sure that many of these uses are erroneous, especially when explicitly casted. Check SDL header files for correct integer types.

(All this shows to me that renaming the types to isize and usize was a good thing, as they were previously incorrectly used as general purpose integers.)

[jpernst]

Indeed, I still find myself typing "int" as pure reflex from other languages. It's a hard habit to break even when you consciously know the implications. Very glad this got changed now before the cement dried, or it would be one of those eternal gotchas that people would be writing books about.

[dcampbell24]

I feel the question "what is proper way to wrap C types whose sizes are platform dependent" needs to be addressed. Currently, this library as well as the sdl1 library are using isize / usize, but improperly.

I propose that if there isn't a Rust type that corresponds to the C type, then the C type should be exposed rather than wrapping it with some fixed size Rust type. What are others thoughts?

[jpernst]

I agree. If a naked "int" is used in a C header, there's no sane choice other than to use libc::c_int for it. No other type will be consistently correct. Fortunately, most C libraries tend to use specified sizes so this doesn't come up as much as it could.

[eagleflo]

There is a Rust type that corresponds to C int on all platforms supported by SDL2: i32.

UNIX on x86_64 uses LP64 while Windows is more conservative with LLP64. Both of these retain int as 32 bits.

[dcampbell24]

Can you provide a reference to support your claim and explain why we don't need to worry about currently unsupported platforms that SDL may support in the future?

To prove this claim we must at least verify it to be true for 32 and 64 bit (where applicable) versions of Windows, Mac OS X, Linux, iOS, and Android[1]

1: https://wiki.libsdl.org/FrontPage

"SDL officially supports Windows, Mac OS X, Linux, iOS, and Android. Support for other platforms may be found in the source code."

For reference, these are two documents mentioning C data models I was able to find:

1. https://www.securecoding.cert.org/confluence/pages/viewpage.action?pageId=4374
2. http://en.wikipedia.org/wiki/64-bit_computing

[eagleflo]

I didn't think my claim would be controversial. You've provided two references that are in agreement with what I wrote above.

Here's how Rust's libc currently defines c_int for different architectures:

~/src/rust (master) $ ag 'type c_int'
src/liblibc/lib.rs
510:                pub type c_int = i32;
726:                pub type c_int = i32;
1011:                pub type c_int = i32;
1238:                pub type c_int = i32;
1453:                pub type c_int = i32;
1883:                pub type c_int = i32;
1986:                pub type c_int = i32;

Here's a C program you can compile and run on any platform you're unsure about:

#include <stdio.h>

int main() {
    printf("%lu bytes\n", sizeof(int));
    return 0;
}

Worry about platforms that SDL might support in the future after they're implemented on SDL side. This is not something a wrapper library should be overtly concerned about. They're currently busy with emscripten and I haven't heard anything about other new or old architectures being even considered. The unofficially supported platforms (mainly BSDs) use LP64 just like other Unices.

I'm ok with keeping libc::c_ints in sdl2-sys part to underline that we're dealing with C ints here, but at least on the high-level Rust wrapper side I think i32 should be used instead due to ergonomics. Recently Rust restored int fallback to i32: https://github.com/rust-lang/rfcs/blob/master/text/0212-restore-int-fallback.md.

[dcampbell24]

I will commit a patch soon to replace a lot of the improper usage of isize by replacing it with i32. However, I also noticed that in enums constants are being cast as isize, which I guess is harmless but seems pointless based on this, but is blocked by this.