glfw: should use u32/i32/etc everywhere instead of usize/isize

emidoots commented

2021-12-01 10:01:26 +00:00

(Migrated from github.com)

Currently it produces an isize width and height, which matches the GLFW int type. But presumably a framebuffer is always positive and there's not a good reason to use signed integers?

Currently it produces an `isize` `width` and `height`, which matches the GLFW `int` type. But presumably a framebuffer is always positive and there's not a good reason to use signed integers?

InKryption commented

2021-12-07 13:09:19 +00:00

(Migrated from github.com)

I've tried investigating, to find any obvious instances where it could be set to a negative value, but it appears to be a pretty barren topic, and there's nothing that stands out in the GLFW source involved with the callback.

It may not be a bad idea to switch to an unsigned type; if anything breaks, then it's something that can be addressed directly, and thus improved.

Though, now thinking about it, I think perhaps switching over to a smaller, fixed integer type, like u32 would be a better for the codebase, being that on 32 bit platforms, c_int is 4 bytes, and usize is 8 bytes, meaning if someone built on a 64 bit platform, they could theoretically make runtime-incompatible software for 32 bit platforms. Yes, an edge case, but as stated in zig zen "Edge cases matter".

I've tried investigating, to find any obvious instances where it could be set to a negative value, but it appears to be a pretty barren topic, and there's nothing that stands out in the GLFW source involved with the callback. It may not be a bad idea to switch to an unsigned type; if anything breaks, then it's something that can be addressed directly, and thus improved. Though, now thinking about it, I think perhaps switching over to a smaller, fixed integer type, like `u32` would be a better for the codebase, being that on 32 bit platforms, `c_int` is 4 bytes, and `usize` is 8 bytes, meaning if someone built on a 64 bit platform, they could theoretically make runtime-incompatible software for 32 bit platforms. Yes, an edge case, but as stated in zig zen "Edge cases matter".

meshula commented

2021-12-08 02:32:33 +00:00

(Migrated from github.com)

+1 on u32. There are already framebuffers 8k wide, so we can imagine a u16 overflowing within a few years. usize and isize are POINTER sized types, not, say window sized types. The C++ std goes a bit awry with size_t which is meant for measuring memory, using it when no pointer measuring is involved. No need to follow in those footsteps ;)

👍 1

emidoots commented

2021-12-09 03:32:54 +00:00

(Migrated from github.com)

@InKryption

being that on 32 bit platforms, c_int is 4 bytes, and usize is 8 bytes, meaning if someone built on a 64 bit platform, they could theoretically make runtime-incompatible software for 32 bit platforms.

I want to make sure I understand this. Are you saying that you believe the current c_int -> usize casts would be invalid on a 32-bit platform (I did not believe so, I'd like to learn/understand why if so) or are you saying that this being usize could lead to someone else making a mistake in their code? Or something else entirely?

@InKryption > being that on 32 bit platforms, c_int is 4 bytes, and usize is 8 bytes, meaning if someone built on a 64 bit platform, they could theoretically make runtime-incompatible software for 32 bit platforms. I want to make sure I understand this. Are you saying that you believe the current c_int -> usize casts would be invalid on a 32-bit platform (I did not believe so, I'd like to learn/understand why if so) or are you saying that this being usize could lead to someone else making a mistake in their code? Or something else entirely?

emidoots commented

2021-12-09 03:53:46 +00:00

(Migrated from github.com)

switching to u32/i32 where appropriate sounds good to me, there are quite a few places where I've used usize and isize as general purpose integers of an unknown size (e.g. to substitute c_int and c_uint) so we will need to vet each one.

switching to u32/i32 where appropriate sounds good to me, there are quite a few places where I've used `usize` and `isize` as general purpose integers of an unknown size (e.g. to substitute `c_int` and `c_uint`) so we will need to vet each one.

InKryption commented

2021-12-09 09:48:18 +00:00

(Migrated from github.com)

@slimsag

I want to make sure I understand this. Are you saying that you believe the current c_int -> usize casts would be invalid on a 32-bit platform (I did not believe so, I'd like to learn/understand why if so) or are you saying that this being usize could lead to someone else making a mistake in their code? Or something else entirely?

Sorry, should have been more clear: I believe there is no issue here, in this particular instance, but it got me thinking about the usage of usize/isize in the codebase, e.g. in window creation, width and height could theoretically overflow, since it's a downcast from usize to c_int (if memory serves me right). Erego, even if there is no issue here, the same change should still be applied here for consistency's sake.

And @meshula also makes a pretty compelling argument.

@slimsag > I want to make sure I understand this. Are you saying that you believe the current c_int -> usize casts would be invalid on a 32-bit platform (I did not believe so, I'd like to learn/understand why if so) or are you saying that this being usize could lead to someone else making a mistake in their code? Or something else entirely? Sorry, should have been more clear: I believe there is no issue here, in this particular instance, but it got me thinking about the usage of `usize`/`isize` in the codebase, e.g. in window creation, width and height could theoretically overflow, since it's a downcast from `usize` to `c_int` (if memory serves me right). Erego, even if there is no issue here, the same change should still be applied here for consistency's sake. And @meshula also makes a pretty compelling argument.

emidoots commented

2021-12-10 21:11:22 +00:00

(Migrated from github.com)

Makes sense, and sounds good to me!

👍 1

meshula commented

2021-12-10 23:12:11 +00:00

(Migrated from github.com)

Bit more info: Specs_guy says var u : usize = something; var i : u64 = @intCast(u64, something) is "free" in terms of processor cycles, but var u : usize = something; var i : u32 = @intCast(u32, something) is undefined on a 64 bit machine.

Bit more info: Specs_guy says `var u : usize = something; var i : u64 = @intCast(u64, something)` is "free" in terms of processor cycles, but `var u : usize = something; var i : u32 = @intCast(u32, something)` is undefined on a 64 bit machine.

emidoots commented

2021-12-10 23:40:20 +00:00

(Migrated from github.com)

@meshula I'm not sure what exactly you mean because the type of something in both of your examples is not clear. I think that you are trying to say @intCast(u32, something) where something is a value greater than a u32 can contain is UB? If so, yeah, that makes sense - this is also why Zig has @truncate(u32, something) if you want to explicitly take e.g. the lower 32 bits of a 64-bit usize

What was not clear to me before this discussion was whether or not usize / isize are appropriate types for a general integer of no specific size, akin to how C and Go have int and uint. And actually, I thought I had seen Andrew Kelley publicly state somewhere that usize and isize were the correct type in this case, but after a few hours of searching I couldn't find it anymore and reading all docs/issues seems to indicate that indeed, it's not, one should use best-effort guesses in such situations (e.g. u32 for framebuffer width/height.) I think my background in using Go/C confused me here, so that's how we ended up with usize and isize everywhere.

@meshula I'm not sure what exactly you mean because the type of `something` in both of your examples is not clear. I _think_ that you are trying to say `@intCast(u32, something)` where `something` is a value greater than a u32 can contain is UB? If so, yeah, that makes sense - this is also why Zig has `@truncate(u32, something)` if you want to explicitly take e.g. the lower 32 bits of a 64-bit `usize` What was not clear to me before this discussion was whether or not `usize` / `isize` are appropriate types for a general integer of no specific size, akin to how C and Go have `int` and `uint`. And actually, I thought I had seen Andrew Kelley publicly state somewhere that `usize` and `isize` were the correct type in this case, but after a few hours of searching I couldn't find it anymore and reading all docs/issues seems to indicate that indeed, it's not, one should use best-effort guesses in such situations (e.g. u32 for framebuffer width/height.) I think my background in using Go/C confused me here, so that's how we ended up with `usize` and `isize` everywhere.

meshula commented

2021-12-10 23:58:44 +00:00

(Migrated from github.com)

Sorry about the unclearness, @InKryption had mentioned "theoretically runtime incompatible interfaces" on 32 bit machines, which sent me down a path of double checking if there was any UB around usize/isize.

The intent of the note was to be adding another supporting argument for u32/i32 interfaces, not explaining zig basics, of which I am definitely not a source of ground truth ;)

something was also unclear; just a placeholder meaning "it doesn't matter what value this is", and Spec_guy also mentioned that @truncate removes the UB, which I neglected to report :)

Sorry about the unclearness, @InKryption had mentioned "theoretically runtime incompatible interfaces" on 32 bit machines, which sent me down a path of double checking if there was any UB around usize/isize. The intent of the note was to be adding another supporting argument for u32/i32 interfaces, not explaining zig basics, of which I am definitely not a source of ground truth ;) `something` was also unclear; just a placeholder meaning "it doesn't matter what value this is", and Spec_guy also mentioned that @truncate removes the UB, which I neglected to report :)

emidoots commented

2021-12-11 00:12:48 +00:00

(Migrated from github.com)

Ahh okay, got it, thanks! Just wanted to make sure I wasn't missing something more subtle in your message :) Appreciate the clarification

InKryption commented

2021-12-11 01:41:54 +00:00

(Migrated from github.com)

I would just like to add: I think it's often more sane to use std.math.clamp than @truncate, because if you downcast a very big integer with only high bits set, it will turn into a very small or value 0 integer.

I would just like to add: I think it's often more sane to use `std.math.clamp` than `@truncate`, because if you downcast a very big integer with only high bits set, it will turn into a very small or value 0 integer.

👍 1

Rows
Columns

glfw: should use u32/i32/etc everywhere instead of usize/isize #114