On Fri, Jan 21, 2022 at 1:32 PM Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> wrote:
> > From: Han-Wen Nienhuys <hanwen@xxxxxxxxxx>
> >
> > 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
