From: Ezekiel Newren Date: Wed, 21 Jan 2026 21:39:24 GMT Subject: Re: [PATCH 01/10] ivec: introduce the C side of ivec Message-ID: In-Reply-To: <08318339-03c3-4068-92fa-7a711bd13da0@gmail.com> On Tue, Jan 20, 2026 at 7:06 AM Phillip Wood wrote: > > Hi Ezekiel > > On 15/01/2026 15:55, Ezekiel Newren wrote: > > On Thu, Jan 8, 2026 at 7:34 AM Phillip Wood 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? If ivec_reserve() isn't suitable then ivec_reserve_exact() should be used instead. > >>> +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? Yes, the Rust implementation will be independent of the C implementation, but will behave the same way. That's why I'm calling it an interoperable vec as opposed to a compatible vec. Rust can't call the C ivec functions and C can't call the Rust ivec functions, but they'll behave the same way. > >>> +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. Maybe I should call this ivec_drop(). Though the notion of explicitly freeing an object in Rust is _almost_ nonsense. The way you free something in Rust is to let it go out of scope. > 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_*. I disagree. IVec isn't a wrapper around an existing struct. ivec is meant to very closely mimic Rust's Vec while guaranteeing interoperability. For things like strbuf I haven't conceived of a solution for that yet. Making ivec diverge from Rust's Vec will result in POLA violations due to different behavior when refactoring an IVec to Vec. > >>> 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 > >> > >> 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. Ah I see what you mean now. I'll experiment with making IVEC_INIT() work like that. One wrinkle is that STRBUF_INIT is a single concrete type whereas IVEC_INIT() is meant for generic types.