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

Zero-cost const D wrapping of non-const C function, sane? #42

Closed
SimonN opened this issue Dec 15, 2017 · 4 comments
Closed

Zero-cost const D wrapping of non-const C function, sane? #42

SimonN opened this issue Dec 15, 2017 · 4 comments

Comments

@SimonN
Copy link
Contributor

SimonN commented Dec 15, 2017

Consider al_get_bitmap_width(ALLEGRO_BITMAP* b). This function doesn't mutate b and should ideally be callable with a const(ALLEGRO_BITMAP)* in usercode.

Allegro 5 doesn't offer a const-correct API, but DAllegro5 has C linkage which looks only at the function's name. const is merely a noble compile-time check in C. :-P I've abused this, and changed my local DAllegro5 to declare like this:

nothrow @nogc extern (C) {
    al_get_bitmap_width(in ALLGERO_BITMAP* b);
}

mixin(ColorWrapper("allegro5.color_ret.", "al_get_pixel",
    "const(ALLEGRO_BITMAP)* bitmap, int x, int y",
    "cast(ALLEGRO_BITMAP*) bitmap, x, y"));

I.e., I added in before the bitmap arguments.

Problems: I assume A5 will never modify *b. A5 doesn't promise this (it is not const-correct) and the wrapper has no chance to detect an eventual change in A5, even though this is unlikely. I didn't get linker errors (Arch 64-bit, building with dmd), that makes me very happy, but who knows if some system out there will check more harshly.

Would you consider such a modification sane for production? Or should const-correct user code cast its const bitmaps to mutable bitmaps before calling DAllegro5?

If this is sane, what functions would be OK to call with const bitmaps to not break any D const promises? Getting height and width, most likely. Maybe also drawing from const bitmap to nonconst bitmap?

@SiegeLord
Copy link
Owner

There's no chance of const causing a link error. I think it's fine to describe the API using D's typesystem in a way that goes beyond the C interfaces.

@SimonN
Copy link
Contributor Author

SimonN commented Dec 16, 2017

Cool, thanks. Before changing DAllegro5 here, I'd like to discuss this with more Allegro 5 devs as a potential change in the Allegro 5 C API: Make the C API const-correct. C has const with sufficient semantics for the D bindings. Only after that, I would make a PR against DAllegro5.

If A5 doesn't get const, then we can still decide whether we want to promise more in the D bindings than the C API promises.

@SimonN
Copy link
Contributor Author

SimonN commented Dec 16, 2017

I've asked this in the D Learn forum and on allegro.cc's development forum.

The problem with the const-accepting API is that it could accept immutable. The compiler assumptions would still break if the C functions changed hidden data inside the opaque bitmaps. In practice, all bitmaps are created mutable by Allegro 5, it's not likely to call the A5 API with immutable.

Without guarantee (which should ideally end up in the C API), maybe we shouldn't change the bindings here, and instead I should continue to cast.

@SimonN
Copy link
Contributor Author

SimonN commented Dec 18, 2017

I believe we shouldn't take const, and instead keep the declarations as they are.

According to Elias in the linked a.cc thread, A5 deliberately doesn't allow const in the C API for bitmaps because it might modify hidden state. Usercode may define its own wrappers that cast const away, and take responsibility to never call the wrappers with immutable.

@SimonN SimonN closed this as completed Dec 18, 2017
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

2 participants