Re: [PATCH v6 14/15] reftable: make reftable_record a tagged union
- From
Han-Wen Nienhuys <hanwen@google.com>
- Date
- Jan 24, 2022, 15:34 UTC
- Message-ID
- <CAFQ2z_PAqW+RS2Znaf2wwOJfdNfkjP1VV84=xaPu_1EAuX+u5w@mail.gmail.com>
- In-Reply-To
- <220121.86fsphjml0.gmgdl@evledraar.gmail.com>
On Fri, Jan 21, 2022 at 1:32 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 16 quoted lines
> > 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.
Show 8 quoted lines
> > + 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.
Show 8 quoted lines
> > + 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.
Show 15 quoted lines
> > -/* 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