On Wed, Feb 28, 2024 at 11:06:52AM +1100, James Liu wrote: > 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. Yeah, fair enough. > Also, since we're unconditionally `memcpy`ing the key into `r->refname` > below, can we avoid the `SWAP(refname, r->refname)` call altogether? No, otherwise `reftable_ref_record_release()` would have already released the underlying pointer of `r->refname` and the call to `REFTABLE_ALLOC_GROW()` would always have to reallocate it. Patrick > > - 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; >
Attachment:
signature.asc
Description: PGP signature
