Re: [PATCH v6 14/15] reftable: make reftable_record a tagged union

[Han-Wen Nienhuys <hanwen@google.com>]

On Fri, Jan 21, 2022 at 1:32 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> > From: Han-Wen Nienhuys <hanwen@google.com>
> >
> > This reduces the amount of glue code, because we don't need a void
> > pointer or vtable within the structure.
> > [...]
> > -     struct reftable_record rec = { NULL };
> > -     reftable_record_from_ref(&rec, &ref);
> > +     struct reftable_record rec = { .type = BLOCK_TYPE_REF,
> > +                                    .u.ref = {
> > +                                            .refname = (char *)name,
> > +                                    } };
> >       return tab->ops->seek_record(tab->table_arg, it, &rec);
> >  }
>
> Both for this & the below don't we prefer to have such assignments on
> their own line? I.e.:

I generally let clang-format do formatting for me, but it looks like
it doesn't want to make a decision about where to put line breaks
here.

I've made the style consistent.
> > +     struct reftable_record clean = {
> > +             .type = typ,
>
> ...only this "type" member, which won't confuse a compiler.
>
> > +     switch (typ) {
>
> ...but actually...

typ vs type is a remnant of the Go origins (type is a reserved word in Go).

I fixed up a couple of places where I add it newly in the commit, but
it makes more sense to change this throughout in a separate commit.

> > +     return clean;
> > +}
>
> ...reading this whole function shouldn't this be a more normal *_init()
> pattern function where the caller just populates the ".type = ", and we
> init the rest here? That would also make the ownership more obvious, and
> if any future API user needs to pass in variable on the heap instead of
> us returning it on the stack here...

this function exists so you can initialize as part of a list of
declarations , eg.

 struct reftable_record rec = reftable_new_record(block_reader_type(br));
 struct strbuf key = STRBUF_INIT;
 int err = 0;
 struct block_iter next = {
  .last_key = STRBUF_INIT,
 };
 int i = binsearch(br->restart_count, &restart_key_less, &args);

with init functions, you have reorganize all of these blocks.


> > -/* zeroes out the embedded record */
> > +/* frees and zeroes out the embedded record */
> >  void reftable_record_release(struct reftable_record *rec);
>
> I didn't follow all the vtable entries, but for these:
>
>     4 matches for ".release =" in buffer: record.c
>         440:        .release = &reftable_ref_record_release_void,
>         582:        .release = &reftable_obj_record_release,
>         925:        .release = &reftable_log_record_release_void,
>        1052:        .release = &reftable_index_record_release,
>
> Some zero'd the data out already, but for
> "reftable_index_record_release" isn't promising this a bug, as we don't
> want to memset() to 0 a strbuf_init()?

Isn't strbuf_release() -which is used- the correct way to clear out a strbuf?

>>       }
>> -
>
>...more stray whitespace...

If you trim this much context, can you provide a line number?

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado