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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jan 20, 2026, 14:06 UTC
Message-ID: <08318339-03c3-4068-92fa-7a711bd13da0@gmail.com>
In-Reply-To: <CAH=ZcbA_HgEO2T2smn4Yg6gf4sm4jrR8A0ek1v9nqsa1MXbRJw@mail.gmail.com>

Hi Ezekiel

On 15/01/2026 15:55, Ezekiel Newren wrote:

Show 17 quoted lines

> 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?

Show 18 quoted lines

>>> +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?

Show 11 quoted lines

>>> +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_*.

Show 23 quoted lines

>>> 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.

Show 6 quoted lines

> ```
> DEFINE_IVEC_TYPE(xrecord_t, xrecord);
> 
> void some_function() {
>      struct IVec_xrecord rec;
>      IVEC_INIT(rec);  // i.e. ivec_init(&rec, sizeof(*rec.ptr);

Thanks

Phillip