aml icon indicating copy to clipboard operation
aml copied to clipboard

Use C11 _Generic instead of void * in public API

Open emersion opened this issue 1 year ago • 6 comments

The public API allows the same functions to be used on different kinds of objects. This makes it pretty error-prone: it's easy to call a function on an invalid type without any compile-time error (either an aml type not accepted by the function in which case it's a runtime error, or a completely foreign type in which case it's just fireworks). C11 _Generic could be used to indicate which types are accepted by a function, e.g:

int _aml_get_fd(const void* obj);

#define aml_get_fd(obj) _Generic(obj, \
    const struct aml*: _aml_get_fd, \
    const struct aml_handler*: _aml_get_fd, \
)(obj)

(Note that in general I am not a fan of functions accepting multiple types of arguments, and would just recommend duplicating the functions into multiple wrappers.)

emersion avatar Sep 18 '24 22:09 emersion

This would probably by an improvement. It could even be done without breaking ABI.

Another approach that I've considered is to approximate type traits via struct members. The interface would still be partially opaque, but the first members for the structs would be exposed. For example:

struct aml_handler {
    struct aml_obj base;
    struct aml_trait_startable startable;
    struct aml_trait_pollable  pollable;
};

int aml_get_fd(struct aml_pollable* obj);

Then you could do...

struct aml_handler handler;
struct aml aml;

...

int handler_fd = aml_get_fd(&handler->pollable);
int aml_fd = aml_get_fd(&aml->pollable);

aml_start(aml_get_default(), &handler->startable);

At least, this would be an interesting experiment in interface design.

any1 avatar Sep 19 '24 07:09 any1

It could even be done without breaking ABI.

Technically speaking using _Generic would be an API break, for instance this would fail compilation after the change:

struct aml_handler *handler = ...;
void *obj = handler;
return aml_get_fd(obj);

Though probably nobody does this in practice.

approximate type traits via struct members

This would be better. I can see these potential downsides:

  • The struct aml_handler struct definition needs to be public. If you want private fields, you need to re-define a private struct wrapping the public one, and cast.
  • There is no way to fish back a struct aml_obj base from struct aml_trait_startable or struct aml_trait_pollable. Maybe that'd be fine for simple getters, but probably wouldn't work for more involved functions. Could be worked around by storing a pointer to the struct aml_obj base inside the "trait" structs perhaps.

emersion avatar Sep 19 '24 10:09 emersion

Technically speaking using _Generic would be an API break, for instance this would fail compilation after the change

I don't mind breaking API as much as ABI.

That being said, I've been wanting to redesign the interface for a while anyway based on some observations that I've made:

  • Passing ownership of userdata to an aml object is almost always the wrong thing to do. What's in userdata should own the aml object, not the other way around. This means that those cleanup functions aren't really needed.
  • Reference counting is needed to be able to safely pass objects between threads, but this is generally not done outside the library itself, so it doesn't need to be exposed in the API.
  • Both of the above features were added as convenience for writing C++ wrappers, but I haven't used this in C++ yet and I don't really intend to.
  • The worker interface is counter-productive. Code that utilises it very often ends up becoming more complicated and error prone than having to deal with multi-threading in the first place.

If/when I do redesign it, I think I'll just do the right thing and duplicate those functions across different types. I did it this way originally because of laziness and a sudden urge to question conventional wisdom.

any1 avatar Sep 19 '24 11:09 any1

I agree with all of the points above.

emersion avatar Sep 19 '24 11:09 emersion

I haven't removed the superfluous things from the API, but at least type safety is addressed: #14

any1 avatar Sep 27 '24 23:09 any1

Very nice, thanks for that!

emersion avatar Sep 28 '24 13:09 emersion