Yeah I am having the landscape of choices mapped out, and I'd like to get a feel for the general opinion to fine tune my heading. "At what degree of which aspects do you guys see the optimum?"
On Mon, Feb 26, 2024 at 10:57:16AM +0100, Pau Espin Pedrol wrote:
As usual, I'd go for opaque. Since opaque means the implementation can be changed, then it doesn't matter much whether you want to use dynamic or static. It can be a mix of both, use whatever fits more in each place.
An opaque API has to be dynamic. There is always a struct foo * that was returned by a dynamic foo_alloc().
But I'd definitely make the lib allocate the structs and have them as opaque pointers.
Ok, poll taken: the benefit of easily modifying the API later weighs more than having to write dozens of foo_get_x() and foo_set_x() and calling functions all the time instead of using plain pointer arithmetic.
Regarding "benefits of iterating over llist", you can simply have an API return the llist pointer and have some API to get the object from the llist per-item pointer which is internally implemented by means of container_of() or a cast or similar.
Yes. But is it really opaque to return an llist_head? Should the API maybe hide the particular list implementation and provide functions for walking the list?
What I'd also like your opinion on is, how far would you take the dynamic-ness? Is it fine to have many detailed allocation "stages", for the benefit of logic and opacity? Example:
I like it when the object itself is the talloc context for its members:
struct codec_list *my_list = codec_list_alloc(ctx); codec_list_add(my_list, item); // <-- use my_list as talloc parent for the new entry
That means I want to allocate an llist_head instance dynamically. Going this way makes a lot more, small dynamic allocations.
(benefit: the logical structure of the data shows very nicely in the talloc reports!)
An alternative would be: use an llist_head that the caller simply has as static member of its state:
struct llist_head my_list; codec_list_add(&my_list, ctx, item); // <-- always provide the talloc ctx separately
It has less separate dynamic allocations, but there are more ways to shoot oneself in the foot, and more ctx arguments all over the API. Also it's not really opaque anymore.
At the moment I want to go for many small dynamic allocations. Fine?
Ah and also, when the osmo_sdp_codec{} is opaque, how would we set up the data structures in codec_mapping.c? I guess with a function that procedurally builds an allocated list? Currently it's an (IMHO nice) const array of pure data: https://gerrit.osmocom.org/plugins/gitiles/osmo-msc/+/master/src/libmsc/code... If struct osmo_sdp_codec is transparent, it can stay this way. If I make it opaque, every part here that says .sdp = {...} would have to be dynamically created via API instead.
(In my current status, osmo_sdp_codec is transparent. If the opinions are strong I'll add effort to make it opaque)
Another idea would be to invent a separate SDP struct in osmo-msc, but that also needs more API to convert one to the other... I don't want that.
Thanks!! ~N