[vendor:vulkan] use uint for c.size_t
Closed this issue · 8 comments
Context
I've been working with odin on a vulkan project lately and I'd noticed that some struct fields in the vendor:vulkan wrapper (like PhysicalDeviceLimits.minMemoryMapAlignment) are of type int, which felt strange.
A quick check with the official vulkan headers showed that the C headers use size_t in that case.
In fact, Odin's vulkan bindings consistently use int whenever size_t is used in the C library.
To the best of my understanding, C's size_t is an unsigned type and odin's int is not.
Odin's own core:c package also defines c.size_t as uint, which is what I'd expect.
I doubt this leads to a problem in any somewhat normal usage scenario and can easily be worked around but it's still technically a bug.
Since Odin utilizes two's complement, this has no tangible effect whatsoever on the behavior of the code.
From a semantic point of view, however I find this sub optimal as these values represent units that are always positive. Using uint would more clearly convey the semantic meaning and would also be more consistent with other parts of Odin (e.g. the core:c package which defines c.size_t as uint).
Expected Behavior
I'd expect the vulkan bindings to use an unsigned type (in particular uint) wherever the original C library uses size_t .
In particular I'd expect the python wrapper to convert size_t to uint instead of to int (see: https://github.com/odin-lang/Odin/blob/master/vendor/vulkan/_gen/create_vulkan_odin_wrapper.py#L67)
Current Behavior
The current vulkan bindings in odin use Odin's int for C's size_t.
Is this an actual practical problem? Is there any case where the range of int is not enough and you need that extra bit that uint provides?
Please note that Odin utiltizes two's complement and thus the "signedness" doesn't matter.
Thank you for the reminder. It seems I completely forgot about that handy feature of Odin, sorry.
I still believe that using int in this case is sub optimal for semantic reasons, but this is definitely not a bug then and shouldn't be marked as such. I'll change the title and description to reflect this.
And what are those semantic reasons? I am not joking, am I asking a serious question.
Maybe it's just me, but when I'm working with libraries, I often look at the type of the variables I'm using just to make sure I use them in a way that makes sense and I forget no corner case. If a variable is of type int, I expect that some state represented by the variable should be interpreted as negative numbers. So if a variable who's name indicates should normally be positive is of type int, I would intuitively expect the negative range to be used either as error codes or that my understanding of the name is incorrect. In my mind using uint would more clearly identify the purpose of these variables.
From your reaction that seems however that this is not quite a common way of thinking about that, so changing this in the library might not be worth it.
Therefore I'll close the issue in a few days if I don't see anyone else affected by this.
Bindings to third party APIs should respect the types declared and used by the API. It may not fix any bugs, but it prevents these kinds of issues from being posted, due to legitimate user concern.
Does vulkan have usage where you'd use -1 even though it's a size_t (making it wrap)? If that's the case int makes sense to me, because you can't do the C style -1 in Odin with a uint.
Not to my knowledge, but it's possible. But if it does, I still think the bindings should reflect reality, and if there is language level friction, that should be resolved by a wrapper. This way, everyone wins, and it's clear where the responsibilities are divided.
I still think int is the best choice here. The cases where uint is used, I cannot think of a case where it requires that extra bit of precision nor would it cause a problem. It is usually better to represent the library accurately, but this is a case where int works fine, and it also makes interacting with Vulkan a lot nicer for Odin.