Re: [PATCH 07/13] reftable/merged: split up initialization and seeking of records
- From
Patrick Steinhardt <ps@pks.im>
- Date
- May 13, 2024, 08:36 UTC
- Message-ID
- <ZkHQ-yAtzm1GNqFY@tanuki>
- In-Reply-To
- <dqnhhjdingkhhwwp5nwut2hsnkkze7m2ls4kscddqxm3txjzbf@5knvdk4vc6eg>
On Fri, May 10, 2024 at 02:18:16PM -0500, Justin Tobler wrote:
Show 7 quoted lines
> On 24/05/08 01:04PM, Patrick Steinhardt wrote: > > To initialize a `struct merged_iter`, we need to seek all subiterators > > to the wanted record and then add their results to the priority queue > > used to sort the records. This logic is split up across two functions, > > `merged_table_seek_record()` and `merged_table_iter()`. The scope of > > Did we mean `merged_iter_init` instead of `merged_table_iter()` here?
Indeed, good catch.
Show 28 quoted lines
> > @@ -246,32 +230,33 @@ static int merged_table_seek_record(struct reftable_merged_table *mt,
> > struct reftable_iterator *it,
> > struct reftable_record *rec)
> > {
> > - struct merged_iter merged = {
> > - .typ = reftable_record_type(rec),
> > - .hash_id = mt->hash_id,
> > - .suppress_deletions = mt->suppress_deletions,
> > - .advance_index = -1,
> > - };
> > - struct merged_iter *p;
> > + struct merged_iter merged, *p;
> > int err;
> >
> > - REFTABLE_CALLOC_ARRAY(merged.subiters, mt->stack_len);
> > + merged_iter_init(&merged, mt);
> > +
> > for (size_t i = 0; i < mt->stack_len; i++) {
> > + reftable_record_init(&merged.subiters[merged.stack_len].rec,
> > + reftable_record_type(rec));
>
> I find it somewhat confusing how we increment the subiterator index
> here. I assume this is because when a record is not found we don't want
> to add it to the index. Might be nice to add a comment here explaining
> this.
>
> Edit: Looks like you address this in the next commit so I guess we are
> good :)Seconded. And yes, as you noticed, this is one of the reasons why I change this in the next patch.
Patrick