search

SiegeLord / DAllegro5

D binding to the Allegro5 game development library
Other
42 stars 15 forks source link

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

Closed SimonN closed 6 years ago

SimonN commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.