Skip to content
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

intmax_t and uintmax_t problematic for some languages FFI #25

Closed
benpye opened this issue May 20, 2016 · 17 comments
Closed

intmax_t and uintmax_t problematic for some languages FFI #25

benpye opened this issue May 20, 2016 · 17 comments

Comments

@benpye
Copy link

benpye commented May 20, 2016

I am currently writing a library for C# wrapping libui for .NET Core. I have generally found the library structured very well for being wrapped for usage in another language. One issue I have come across is the usage of intmax_t and uintmax_t. C# has no equivalent for these types (and I suspect other languages also have this limitation), and as such I am currently assuming that they are 64 bit integers, but this could not be true. If the API could instead expose these as some fixed width integer then this would be much easier to deal with, especially when wanting to target any Windows, Linux or OS X system.

@andlabs
Copy link
Owner

andlabs commented May 20, 2016

Yes, I've been thinking about that as well, though more for "warning: you're converting a larger type to a smaller type" reasons. I wonder if I could just make them int and unsigned int instead (or get rid of uintmax_t entirely and just use int). I guess the better question is how reasonable it would be for the range of a list to approach 2 ^ (2 ^ sizeof (size_t)) entries...

@Someguynamedpie
Copy link

Could use types like uint32_t or uint64_t

@pcwalton
Copy link
Contributor

In Rust I'm just mapping these to i64 and u64, as that is what the Rust libc crate defines intmax_t and uintmax_t as.

@andlabs
Copy link
Owner

andlabs commented May 22, 2016

I'm tempted to just use int or maybe long, under the rationale that nothing should really exceed 2 billion objects anyway. I'm not sure if I'd be comfortable pinning to int64 if we ever move beyond 64-bit... I'm open to arguments for or against anything though :)

@zevv
Copy link

zevv commented May 24, 2016

For the Combobox, Box and Tab indices an unsigned int should be just fine

The slider might benefit from using a floating point type for more flexibility, but I'm not sure of all the various OS backends support this.

@billyquith
Copy link

How does it deal with size_t?

Could you do something like this? http://stackoverflow.com/a/9203792/3233

@andlabs
Copy link
Owner

andlabs commented Jun 6, 2016

So what should it be, int or long?

size_t might pose the same problems intmax_t pose; I'm not 100% sure. Anyone? @benpye?

@billyquith
Copy link

billyquith commented Jun 6, 2016

So the problem is that int and long are 32 and 64 bits for C#, regardless (word size is only relevant in the JIT compiler). So, how about:

Define in your LibUI C config header:

#ifndef UI_SIZE_T
#define UI_SIZE_T size_t
#endif
typedef UI_SIZE_T ui_size_t;

Then in cmake config you allow a variable BUILD_SIZE_T which is defined if the user wants to compile LibUI with a different type for size_t. The default works for C, C++, etc. For C# you generate a project with:

cmake .. -G xxx  -D BUILD_SIZE_T=uint64_t

In cmake config:

if(BUILD_SIZE_T)
    add_definitions("UI_SIZE_T=${BUILD_SIZE_T}")
endif()

Then ui_size_t is always passed and returned as uint64_t. This may produce some type data loss warning when this is used but this should be safe as long as the client code is aware of whether LibUI is compiled as 32 or 64 bit.

@benpye
Copy link
Author

benpye commented Jun 6, 2016

You can use the native integer size in C# with IntPtr but it's certainly not a brilliant solution. Using a fixed length integer would be the easiest for C# P/Invoke, either uint32_t or uint64_t would work I suppose, though the former would avoid a performance penalty on 32 bit platforms. The workaround you've linked to @billyquith is somewhat like what Microsoft does in corefx, however it would be nice to avoid that as it would mean you could have one binding library for .NET everywhere (Windows, Linux, OS X, FreeBSD on x86, x86_64, ARM, etc...).

@billyquith
Copy link

either uint32_t or uint64_t would work I suppose, though the former would avoid a performance penalty on 32 bit platforms

It's up to your which one you choose, I just chose 64 bit as it obviously supports 64 bit platforms, which are probably the default now. But I suspect it is unlikely sizes will exceed the range of max uint32_t. However if handles (e.g. opaque pointers) are necessary then 64 bit would be handy, or perhaps another type could be specified for this, but if/when necessary. As you say, intPtr could also be used but it a load of hassle for little gain. I think this solution is the best compromise and allows each wrapper the option to chose an appropriate solution.

@benpye
Copy link
Author

benpye commented Jun 6, 2016

If we have opaque pointers, then that really is when IntPtr is suitable. As
you say, uint32_t vs uint64_t is a somewhat arbitrary choice.

On Mon, 6 Jun 2016 at 12:22 Billy Quith notifications@github.com wrote:

either uint32_t or uint64_t would work I suppose, though the former would
avoid a performance penalty on 32 bit platforms

It's up to your which one you choose, I just chose 64 bit as it obviously
supports 64 bit platforms, which are probably the default now. But I
suspect it is unlikely sizes will exceed the range of max uint32_t. However
if handles (e.g. opaque pointers) are necessary then 64 bit would be handy,
or perhaps another type could be specified for this, but if/when necessary.
As you say, intPtr could also be used but it a load of hassle for little
gain. I think this solution is the best compromise and allows each wrapper
the option to chose an appropriate solution.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#25 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAbA_yoEgzvFwErzJZs6BmogQoHAxprOks5qJAMJgaJpZM4IjNMl
.

@andlabs
Copy link
Owner

andlabs commented Jun 9, 2016

Leaning toward int, with a requirement that sizeof(int) >= 32 bits, as having 2 billion objects is already unrealistic and poses major usability problems. Design for the trivial case, after all. Any objections?

This issue must be closed before an Alpha 4 can be released. I expect to tackle this and #88 in the short term.

@billyquith
Copy link

Do you mean as an addition to the change I suggested, or instead of? My suggestion seems to fix the C# issue.

@andlabs
Copy link
Owner

andlabs commented Jun 9, 2016

Which comment has the suggestion you are referring to?

@billyquith
Copy link

The one 3days ago.

@andlabs
Copy link
Owner

andlabs commented Jun 9, 2016

Ah.

There are a few places where size_t is used right now for "size of C array" (such as uiDrawStrokeParams.Dashes/NumDashes). Every platform I know of has its own convention for dealing with array sizes. For instance, both C# and Go use int (though the meaning of int differs between the languages) and Rust uses usize. So my advice for bindings is to just use that where they see size_t.

But more important is that I can't just replace intmax_t with size_t because size_t is unsigned; I need to be able to represent -1, so that won't do. ssize_t is nonstandard.

I'll probably just go with int.

@andlabs
Copy link
Owner

andlabs commented Jun 14, 2016

Switched all cases of intmax_t and uintmax_t in the public API to int. Hope this helps!

@andlabs andlabs closed this as completed Jun 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants