On 21/01/2026 21:39, Ezekiel Newren wrote:

On Tue, Jan 20, 2026 at 7:06 AM Phillip Wood [off-list ref] wrote:

quoted

Hi Ezekiel

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

quoted

On Thu, Jan 8, 2026 at 7:34 AM Phillip Wood [off-list ref] wrote:

quoted

quoted

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

If some C code that pushes one element at a time to an array using 
ALLOC_GROW() is converted to use an ivec then we don't want to change 
the code behaves - that means it should grow the array in the same way. 
I don't see how the suggestion to use ivec_reserve_exact() helps in that 
situation. What is the advantage in having a different growth 
characteristic?

quoted

quoted

quoted

quoted

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

Interesting - I'm curious what the advantage of that is over having rust 
call the C implementation? I can see you wouldn't want to be calling 
into C for each ivec.push() call, but checking if there is room to push 
the new element in rust and calling into C to extend the vector if not 
should be reasonable and then you don't have to re-implement everything 
in rust.

quoted

quoted

quoted

quoted

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

Indeed - which means this wont be a public function in rust and so why 
do we worry about naming it ivec_clear()? At least ivec_drop() does not 
conflict with any of the standard function suffixes that we're already 
using in git.

quoted

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.

So just because it is a new stuct it shouldn't have to follow the 
existing naming conventions?

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<your_type_here> to Vec<your_type_here>.

On the other hand, vec.reset() does not exist so you'd get a compiler 
error if you forgot to rename those calls when changing from IVec to Vec 
and the rust code wouldn't be calling ivec.clear(). I'm not sure citing 
POLA concerns is very convincing as ivec_free() in C is a POLA violation 
for anyone familiar with git's code base so it's not like there's a 
choice that avoids that concern.

Thanks

Phillip

quoted

quoted

quoted

quoted

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

quoted

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

If you can get it to work that would be great, but I can't think of a 
way of getting it to work for a generic type.

Thanks

Phillip