Re: [PATCH v2 4/5] config: add git_configset_alloc() and git_configset_clear_and_free()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux