Re: [PATCH 01/10] ivec: introduce the C side of ivec

>
> Hi Ezekiel
>
> On 15/01/2026 15:55, Ezekiel Newren wrote:
> > On Thu, Jan 8, 2026 at 7:34 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >>> +void ivec_reserve(void *self_, size_t additional)
> >>> +{
> >>> +     struct IVec_c_void *self = self_;
> >>> +
> >>> +     size_t growby = 128;
> >>> +     if (self->capacity > growby)
> >>> +             growby = self->capacity;
> >>> +     if (additional > growby)
> >>> +             growby = additional;
> >>
> >> This growth strategy differs from both ALLOC_GROW() and
> >> XDL_ALLOC_GROW(), if there isn't a good reason for that we should
> >> perhaps just use ALLOC_GROW() here.
> >
> > XDL_ALLOW_GROW() can't be used because the pointer is always a void*
> > in this function.
>
> Oh right. I'm not sure that's not a reason to use a different growth
> strategy though. The minimum size of 128 elements is probably good for
> the xdiff code that creates arrays with one element per line but if this
> is supposed to be for general use it is going to waste space when we're
> allocating a lot of small arrays. ALLOC_GROW() uses alloc_nr() to
> calculate the new side so perhaps we could use that here?

> >>> +void ivec_push(void *self_, const void *value)
> >>> +{
> >>> +     struct IVec_c_void *self = self_;
> >>> +     void *dst = NULL;
> >>> +
> >>> +     if (self->length == self->capacity)
> >>> +             ivec_reserve(self, 1);
> >>> +
> >>> +     dst = (uint8_t*)self->ptr + self->length * self->element_size;
> >>> +     memcpy(dst, value, self->element_size);
> >>
> >> If self->element_size was a compile time constant the compiler could
> >> easily optimize this call away. I'm not sure that is easy to achieve though.
> >
> > The problem is that I didn't want all of ivec to be macros that looked
> > like function calls. I wanted to minimize use of macros so that it was
> > easier to port and verify that the Rust implementation matches the
> > behavior of the C implementation.
>
> I think that's a reasonable concern. So is the plan to have a parallel
> rust implementation of these functions rather than call the C
> implementation from rust?

> >>> +void ivec_free(void *self_)
> >>
> >> Normally we'd call a like this that free the allocations and
> >> re-initializes the members ivec_clear()
> >
> > In Rust Vec.clear() means to set length to zero, but leaves the
> > allocation alone. The reason why I'm zeroing the struct is to help
> > avoid FFI issues. If not zero then what should the members be set to,
> > to indicate that using the struct is not valid anymore? In Rust an
> > object is freed when it goes out of scope and _cannot_ be accessed
> > afterward.

> I'm aware that Vec::clear() has different semantics (it does what
> strbuf_reset() does). That's unfortunate but this function has different
> semantics to all the other *_free() functions in git. Our coding
> guidelines say
>
>   - There are several common idiomatic names for functions performing
>     specific tasks on a structure `S`:
>
>      - `S_init()` initializes a structure without allocating the
>        structure itself.
>
>      - `S_release()` releases a structure's contents without freeing the
>        structure.
>
>      - `S_clear()` is equivalent to `S_release()` followed by `S_init()`
>        such that the structure is directly usable after clearing it. When
>        `S_clear()` is provided, `S_init()` shall not allocate resources
>        that need to be released again.
>
>      - `S_free()` releases a structure's contents and frees the
>        structure.
>
> As we write more rust code and so wrap more of our existing structs
> we're going to be wrapping C code that uses the definitions above so I
> think we should do the same with struct IVec_*.

> >>> diff --git a/compat/ivec.h b/compat/ivec.h
> >>> new file mode 100644
> >>> index 0000000000..654a05c506
> >>> --- /dev/null
> >>> +++ b/compat/ivec.h
> >>> @@ -0,0 +1,52 @@
> >>> +#ifndef IVEC_H
> >>> +#define IVEC_H
> >>> +
> >>> +#include <git-compat-util.h>
> >>
> >> It would be nice to have some documentation in this header, see the
> >> examples in strvec.h and hashmap.h
> >>
> >>> +#define IVEC_INIT(variable) ivec_init(&(variable), sizeof(*(variable).ptr))
> >>
> >> This is a bit cumbersome to use compared to our usual *_INIT macros. I'm
> >> struggling to see how we can make it nicer though as DEFINE_IVEC_TYPE
> >> cannot define a per-type initializer macro and I we cannot initialize
> >> the element size without knowing the type.
> >
> > I don't see what's cumbersome about it. Maybe an example use case
> > would clarify things.
>
> It is cumbersome because it separates the initialization from the
> declaration. Normally our *_INIT macros are initializer lists so we can
> write
>
>         struct strbuf = STRBUF_INIT;
>
> which keeps the declaration and initialization together. Although
> they're on adjacent lines in your example in real code the
> initialization likely to be separated from the declaration by other
> variable declarations.