On Wed Feb 14, 2024 at 6:46 PM AEDT, Patrick Steinhardt wrote: > This refactoring is safe to do because all functions that assigning to > the refname will first call `release_reftable_record()`, which will zero > out the complete record after releasing memory. s/release_reftable_record/reftable_ref_record_release > Furthermore, with this change we now perform a mostly constant number of > allocations when iterating. That's awesome! > + SWAP(refname, r->refname); > + SWAP(refname_cap, r->refname_cap); > reftable_ref_record_release(r); > + SWAP(refname, r->refname); > + SWAP(refname_cap, r->refname_cap); What do you think about reversing the order of the `SWAP` arguments in the last two invocations? If my understanding is correct that we're preserving the `refname` and `refname_cap` fields so we can set them back into a freshly initialised `r`, reversing the args might make that intent a bit clearer. Also, since we're unconditionally `memcpy`ing the key into `r->refname` below, can we avoid the `SWAP(refname, r->refname)` call altogether? > - assert(hash_size > 0); > - > - r->refname = reftable_malloc(key.len + 1); > + REFTABLE_ALLOC_GROW(r->refname, key.len + 1, r->refname_cap); > memcpy(r->refname, key.buf, key.len); > r->refname[key.len] = 0;
