-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Wrong assumptions about C types #823
Comments
There's a worse example: |
You may think, why doesn't everything fall apart when things like this are used? Well... one function argument (not sure if last one or first one) is "allowed" to have wrong size. Same position in stack, little-endian architecture... |
I agree. We could have top-level aliases like @waj thoughts? |
Yes, like the |
I'm not sure I'd introduce them to the toplevel, I wouldn't mind having to write |
+1 for me. I also agree with @jhass too. |
+1 not polluting the toplevel seems right, yet, I would allow just May be we should avoid other types in a |
I just added the type aliases (69c6254) and changed a few declarations. Change all the existing declarations might be a tedious job, so I suggest we fix them as needed. I'll update soon the function definitions of |
After the above commit, in which I went through all libs to make sure they are mapped to the correct types, and this one I'm closing this. @BlaXpirit thanks for brining this issue. I believe writing C bindings now will much easier and less error-prone. |
…g two pointers. ptrdiff_t is used for pointer arithmetic and array indexing, if negative values are possible. Refs crystal-lang#823, crystal-lang#4589.
…g two pointers. ptrdiff_t is used for pointer arithmetic and array indexing, if negative values are possible. Refs crystal-lang#823, crystal-lang#4589.
In Crystal's codebase wrong assumptions are made about C types. The type
int
is just mapped asInt32
, etc. The same assumptions are recommended in the documentation (introduction).Why am I saying this is wrong? There are very few, if any, C types that have their exact byte size specified in the standard.
What does the C++ standard state the size of int, long type to be?
Of course, there are some assumptions that are true for current major systems (say, Windows, Mac and Linux), so let's list them.
Signed-ness won't be mentioned here, because it should be obvious.
char
is 1 byteshort
is 2 bytesint
is 4 byteslong long
is 8 bytessize_t
is the same number of bits as the OS bit-ness (4 or 8 bytes)long
size_t
float
is 4 bytesdouble
is 8 bytesReference
(Here are some of these things implemented in code)
I've definitely seen a few cases where one of
size_t
/long
is mapped as one ofInt32
/Int64
in Crystal's codebase. So, many things are broken now, not just in perspective.Short-term fix: make sure the definitions of
SizeT
andLongT
are correct and always use these.This will need to be done if we expect Crystal to work on Windows.
But, I don't think it's a good idea in general to directly use Crystal's
IntXX
types for C bindings, because that works only by coincidence with the current de-facto standards. So if Crystal wants to support another platform has has some different choices, not only will there be a ton of work required to fix up the Crystal compiler for them, but additionally (almost) every library that uses C bindings will break. You probably realize that this is not something that can be solved reasonably.My suggestion is to simply provide aliases for every C numeric type, even if they are trivial. Trivial for now, that is, because they easily may change in the future. In addition, less thinking is required to use straightforwardly named aliases.
A great example of such aliases
The sooner this is done, the better.
The text was updated successfully, but these errors were encountered: