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.
