| Message ID | 20240906222116.270196-4-calvinwan@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce libgit-rs, a Rust wrapper around libgit.a | expand |
On Fri, Sep 6, 2024 at 6:21 PM Calvin Wan <calvinwan@google.com> wrote: > From: Josh Steadmon <steadmon@google.com> > > Add git_configset_alloc() and git_configset_clear_and_free() functions > so that callers can manage config_set structs on the heap. This also > allows non-C external consumers to treat config_sets as opaque structs. > > Co-authored-by: Calvin Wan <calvinwan@google.com> > Signed-off-by: Calvin Wan <calvinwan@google.com> Almost all these patches suffer from some sort of missing Josh trailer, so I'll stop mentioning it now. > diff --git a/config.h b/config.h > @@ -472,6 +472,11 @@ struct config_set { > +/** > + * Alloc a config_set > + */ > +struct config_set *git_configset_alloc(void); Should this documentation string mention that git_configset_alloc() does _not_ initialize the configset? Alternatively, should this function also initialize it as a convenience (and mention so in the documentation)?
On Fri, Sep 06, 2024 at 07:24:22PM -0400, Eric Sunshine wrote: > On Fri, Sep 6, 2024 at 6:21 PM Calvin Wan <calvinwan@google.com> wrote: > > From: Josh Steadmon <steadmon@google.com> > > > > Add git_configset_alloc() and git_configset_clear_and_free() functions > > so that callers can manage config_set structs on the heap. This also > > allows non-C external consumers to treat config_sets as opaque structs. > > > > Co-authored-by: Calvin Wan <calvinwan@google.com> > > Signed-off-by: Calvin Wan <calvinwan@google.com> > > Almost all these patches suffer from some sort of missing Josh > trailer, so I'll stop mentioning it now. > > > diff --git a/config.h b/config.h > > @@ -472,6 +472,11 @@ struct config_set { > > +/** > > + * Alloc a config_set > > + */ > > +struct config_set *git_configset_alloc(void); > > Should this documentation string mention that git_configset_alloc() > does _not_ initialize the configset? Alternatively, should this > function also initialize it as a convenience (and mention so in the > documentation)? I'd think so. I don't quite see the point in splitting this up into two separate function calls. Is there ever a case where one wants to allocate the configset, but not use it? Patrick
On 10/09/2024 07:41, Patrick Steinhardt wrote: > Is there ever a case where one wants to > allocate the configset, but not use it? That was my thought too - I suggested providing git_configset_new() that would allocate and initialize a config set in my response to the last round [1]. It is good to see that the struct in now namespaced in the next patch but separating out allocation and initialization makes the api harder to use than it needs to be. I'd also like to see git_configset_clear_and_free() become git_configset_free(). Best Wishes Phillip [1] https://lore.kernel.org/git/47b18fa4-f01b-4f42-8d04-9e145515ccc1@gmail.com
Phillip Wood <phillip.wood123@gmail.com> writes: > On 10/09/2024 07:41, Patrick Steinhardt wrote: >> Is there ever a case where one wants to >> allocate the configset, but not use it? > > That was my thought too - I suggested providing git_configset_new() > that would allocate and initialize a config set in my response to the > last round [1]. It is good to see that the struct in now namespaced in > the next patch but separating out allocation and initialization makes > the api harder to use than it needs to be. I'd also like to see > git_configset_clear_and_free() become git_configset_free(). Makes sense.
On Tue, Sep 10, 2024 at 1:50 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > On 10/09/2024 07:41, Patrick Steinhardt wrote: > > Is there ever a case where one wants to > > allocate the configset, but not use it? > > That was my thought too - I suggested providing git_configset_new() that > would allocate and initialize a config set in my response to the last > round [1]. It is good to see that the struct in now namespaced in the > next patch but separating out allocation and initialization makes the > api harder to use than it needs to be. I'd also like to see > git_configset_clear_and_free() become git_configset_free(). > > Best Wishes > > Phillip > > [1] > https://lore.kernel.org/git/47b18fa4-f01b-4f42-8d04-9e145515ccc1@gmail.com Agreed, it doesn't make sense to have both alloc() and init() when they can be combined and same with renaming git_configset_clear_and_free() to git_configset_free().
On 2024.09.06 19:24, Eric Sunshine wrote: > On Fri, Sep 6, 2024 at 6:21 PM Calvin Wan <calvinwan@google.com> wrote: > > From: Josh Steadmon <steadmon@google.com> > > > > Add git_configset_alloc() and git_configset_clear_and_free() functions > > so that callers can manage config_set structs on the heap. This also > > allows non-C external consumers to treat config_sets as opaque structs. > > > > Co-authored-by: Calvin Wan <calvinwan@google.com> > > Signed-off-by: Calvin Wan <calvinwan@google.com> > > Almost all these patches suffer from some sort of missing Josh > trailer, so I'll stop mentioning it now. > > > diff --git a/config.h b/config.h > > @@ -472,6 +472,11 @@ struct config_set { > > +/** > > + * Alloc a config_set > > + */ > > +struct config_set *git_configset_alloc(void); > > Should this documentation string mention that git_configset_alloc() > does _not_ initialize the configset? Alternatively, should this > function also initialize it as a convenience (and mention so in the > documentation)? Yes, in V4 this will both alloc and init the configset (and the function has been moved to the shim library).
diff --git a/config.c b/config.c index 6421894614..444db8de79 100644 --- a/config.c +++ b/config.c @@ -2324,6 +2324,11 @@ static int config_set_element_cmp(const void *cmp_data UNUSED, return strcmp(e1->key, e2->key); } +struct config_set *git_configset_alloc(void) +{ + return xmalloc(sizeof(struct config_set)); +} + void git_configset_init(struct config_set *set) { hashmap_init(&set->config_hash, config_set_element_cmp, NULL, 0); @@ -2353,6 +2358,12 @@ void git_configset_clear(struct config_set *set) set->list.items = NULL; } +void git_configset_clear_and_free(struct config_set *set) +{ + git_configset_clear(set); + free(set); +} + static int config_set_callback(const char *key, const char *value, const struct config_context *ctx, void *cb) diff --git a/config.h b/config.h index 54b47dec9e..074c85a788 100644 --- a/config.h +++ b/config.h @@ -472,6 +472,11 @@ struct config_set { struct configset_list list; }; +/** + * Alloc a config_set + */ +struct config_set *git_configset_alloc(void); + /** * Initializes the config_set `cs`. */ @@ -520,6 +525,11 @@ int git_configset_get_string_multi(struct config_set *cs, const char *key, */ void git_configset_clear(struct config_set *cs); +/** + * Clears and frees a heap-allocated `config_set` structure. + */ +void git_configset_clear_and_free(struct config_set *cs); + /* * These functions return 1 if not found, and 0 if found, leaving the found * value in the 'dest' pointer.