Re: [PATCH 01/10] ivec: introduce the C side of ivec
- From
Ezekiel Newren <ezekielnewren@gmail.com>
- Date
- Jan 15, 2026, 15:55 UTC
- Message-ID
- <CAH=ZcbA_HgEO2T2smn4Yg6gf4sm4jrR8A0ek1v9nqsa1MXbRJw@mail.gmail.com>
- In-Reply-To
- <0437b899-5a36-4499-a30a-c2a074a80f7e@gmail.com>
On Thu, Jan 8, 2026 at 7:34 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
Show 13 quoted lines
> > diff --git a/compat/ivec.c b/compat/ivec.c
> > new file mode 100644
> > index 0000000000..0a777e78dc
> > --- /dev/null
> > +++ b/compat/ivec.c
> > @@ -0,0 +1,113 @@
> > +#include "ivec.h"
> > +
> > +struct IVec_c_void {
>
> We normally use all lower case names for structs but as this is shared
> with rust it maybe makes sense to use CamelCase so the names are the
> same in both languages.My preference would be all lowercase, but cbindgen insists on using the same casing as was used in Rust. I don't think there's a way to make cbindgen use all lowercase for structs.
Show 14 quoted lines
> > + void *ptr;
> > + size_t length;
> > + size_t capacity;
> > + size_t element_size;
> > +};
> > +
> > +static void _set_capacity(void *self_, size_t new_capacity)
> > +{
> > + struct IVec_c_void *self = self_;
>
> Passing any of the ivec variants defined below to this function invokes
> undefined behavior because we're not casting the pointer back to the
> orginal type. However I think on the platforms we care about
> sizeof(void*) == sizeof(T*) for all T so maybe we can look the other way.If someone finds that this code does not work because of this assumption I'd like to know. But I can't fathom a case where it wouldn't work.
Show 22 quoted lines
> > +
> > + if (new_capacity == self->capacity) {
> > + return;
> > + }
> > + if (new_capacity == 0) {
> > + free(self->ptr);
> > + self->ptr = NULL;
> > + } else {
> > + self->ptr = realloc(self->ptr, new_capacity * self->element_size);
> > + }
> > + self->capacity = new_capacity;
>
> Not if realloc() returns NULL. We should check for that, probably by
> using xrealloc().
>
> > +void ivec_zero(void *self_, size_t capacity)
> > +{
> > + struct IVec_c_void *self = self_;
> > +
> > + self->ptr = calloc(capacity, self->element_size);
>
> We should be handling allocation failures here probably by using xcalloc().I've changed it to xrealloc() similar for the calloc() call.
Show 13 quoted lines
> > +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.
Show 13 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.
> > +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.
Show 19 quoted lines
> > +{
> > + struct IVec_c_void *self = self_;
> > +
> > + free(self->ptr);
> > + self->ptr = NULL;
> > + self->length = 0;
> > + self->capacity = 0;
> > + // DO NOT MODIFY element_size!!!
> > +}
> > +
> > +void ivec_move(void *src_, void *dst_)
> > +{
> > + struct IVec_c_void *src = src_;
> > + struct IVec_c_void *dst = dst_;
>
> Maybe we should add
>
> if (src->element_size != dst->element_size)
> BUG("moving incompatible arrays");I'll do that.
Show 8 quoted lines
> > + > > + ivec_free(dst); > > + dst->ptr = src->ptr; > > + dst->length = src->length; > > + dst->capacity = src->capacity; > > + // DO NOT MODIFY element_size!!! > > As the element sizes must match maybe *dst = *src would be clearer?
That seems fine.
Show 26 quoted lines
> > + > > + src->ptr = NULL; > > + src->length = 0; > > + src->capacity = 0; > > + // DO NOT MODIFY element_size!!! > > +} > > 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.
``` DEFINE_IVEC_TYPE(xrecord_t, xrecord);
void some_function() {
struct IVec_xrecord rec;
IVEC_INIT(rec); // i.e. ivec_init(&rec, sizeof(*rec.ptr);// use concrete functions to manipulate vector or access the array directly via ptr } ```
IVEC_INIT() should be used on the concrete type.
Show 22 quoted lines
> > +
> > +#ifndef CBINDGEN
> > +#define DEFINE_IVEC_TYPE(type, suffix) \
> > +struct IVec_##suffix { \
> > + type* ptr; \
> > + size_t length; \
> > + size_t capacity; \
> > + size_t element_size; \
> > +}
>
> I wonder if we want to define type safe inline safe wrappers for the
> ivec_* functions here. I think the only functions where the element type
> matters are ivec_move() and ivec_push(), for the others like
> ivec_zero(), ivec_reserve() and ivec_free() the element type does not
> matter. ivec_push() would certainly be easier to use with a wrapper as
> means we can avoid forcing the caller to take the address of the value.
>
> static inline ivec_##suffix##_push(struct IVec_##suffix *self, type
> value) { \
> const void *ptr = &value; \
> ivec_push(self, ptr); \
> }I turned ivec_push() into a macro, but the rest will remain as concrete functions.