From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 13 Oct 2025 11:10:40 GMT
Subject: Re: [PATCH 6/9] reftable/stack: return stack segments directly
Message-ID: <aOzeMLVx6P_AbxCQ@pks.im>
In-Reply-To: <CAOLa=ZSfiD4KTSMyc2WW4oC2zyDO9CHmUWwnws3GhCdbtL4rAA@mail.gmail.com>

On Mon, Oct 13, 2025 at 02:01:15AM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> >> diff --git a/reftable/stack.c b/reftable/stack.c
> >> index f91ce50bcd..9d9326ce0e 100644
> >> --- a/reftable/stack.c
> >> +++ b/reftable/stack.c
> >> @@ -1639,29 +1640,29 @@ static uint64_t *stack_table_sizes_for_compaction(struct reftable_stack *st)
> >>
> >>  	REFTABLE_CALLOC_ARRAY(sizes, st->merged->tables_len);
> >>  	if (!sizes)
> >> -		return NULL;
> >> +		return REFTABLE_OUT_OF_MEMORY_ERROR;
> >>
> >>  	for (size_t i = 0; i < st->merged->tables_len; i++)
> >>  		sizes[i] = st->tables[i]->size - overhead;
> >>
> >> -	return sizes;
> >> +	*seg = suggest_compaction_segment(sizes, st->merged->tables_len,
> >> +					  st->opts.auto_compaction_factor);
> >> +	reftable_free(sizes);
> >> +
> >> +	return 0;
> >>  }
> >>
> >>  int reftable_stack_auto_compact(struct reftable_stack *st)
> >>  {
> >>  	struct segment seg;
> >> -	uint64_t *sizes;
> >> +	int err = 0;
> >
> > The initialization is unnecessary.
> >
> 
> True, but I always think its nicer to define the base state like this.
> This makes it less error prone when/if the code is modified in the
> future. But doesn't really matter, so let me remove it.

The counterargument to this is that it doesn't give us any nice compiler
warnings if a specific return forgets to set the variable, so we may
return successfully by accident. If you leave this uninitialized then
the compiler warns you for every early return where you don't initialize
the variable. For all I can tell this warning is also quite reliable.

Patrick