On 2024.08.13 10:48, phillip.wood123@xxxxxxxxx wrote: > Hi Josh > > On 12/08/2024 22:39, Josh Steadmon wrote: > > On 2024.08.12 10:10, Phillip Wood wrote: > > > Hi Josh > > > > > > Do we really need to add this code to config.c rather than handling it in > > > the wrapper layer in the next patch? > > > > > > Looking ahead I wonder how useful it is to users of the library to separate > > > out allocation from initialization. A function that allocates and > > > initializes a configset would be more convenient and harder to misuse. > > > Calling release functions *_free() rather than *_clear_and_free() would be > > > more convenient as well. I also noticed that the data types are not > > > namespaced when they are exported. So perhaps we could drop this patch and > > > add the following to the next patch. > > > > > > /* > > > * Namespace data types as well as functions and ensure consistent > > > * naming of data types and the functions that operate on them. > > > */ > > > struct libgit_configset { > > > struct config_set set; > > > }; > > > > > > struct libgit_configset *libgit_configset_new() { > > > struct libgit_configset *set = xmalloc(sizeof(*set)); > > > > > > git_configset_init(&set->set); > > > return set; > > > } > > > > > > void libgit_configset_free(struct libgit_configset *set) { > > > git_configset_clear(&set->set); > > > free(set); > > > } > > > > Hmmm I see your point, but I am also hoping to keep the symbol export > > shim as small as possible, so that we can try to autogenerate it rather > > than add entries by hand. > > That's a nice aim - how do you plan to address namespacing data types with > that approach? The autogenerator would need to know "struct config_set" > wants to be wrapped as "struct libgit_configset" to ensure we end up with > consistent naming for the data type and its functions. We'd also still want > to make sure we end up with an ergonomic api which means one function to > allocate and initialize each data type, not separate functions for > allocation and initialization. > > > However, if people feel strongly that we don't > > want to add helper functions like *_alloc() or *_free() for types that don't > > already have them upstream, perhaps we can just put them in a separate > > rust-helpers.c file or something. > > If we're not using them upstream they're just clutter as far as git is > concerned. Having said that if you get the autogeneration working well so > the output is usable without a lot of manual tweaking I can see an argument > for having these functions upstream. > > Best Wishes > > Phillip For V4 I'm going to drop the ambition to autogenerate the shim library, and as such there's no longer a reason to keep helper functions out of the shim library. So I've moved the _alloc and _free functions into the shim. For namespacing data types: for now I'm just letting the compiler infer an opaque "struct libgit_config_set" and manually cast back to "struct config_set" when we cross the boundary from the shim library back to libgit.a. I'm not sure this is the right approach, but it avoids having to wrap a single pointer in a separate struct.