Subject: Re: [PATCH 08/10] bloom: split 'get_bloom_filter()' in two

From: Taylor Blau <[email-masked]>
Date: Tue, 4 Aug 2020 16:12:47 -0400

On Tue, Aug 04, 2020 at 09:00:31AM -0400, Derrick Stolee wrote:
> On 8/3/2020 2:57 PM, Taylor Blau wrote:
> > 'get_bloom_filter' takes a flag to control whether it will compute a
> > Bloom filter if the requested one is missing. In the next patch, we'll
> > add yet another flag to this method, which would force all but one
> > caller to specify an extra 'NULL' parameter at the end.
> >
> > Instead of doing this, split 'get_bloom_filter' into two functions:
> > 'get_bloom_filter' and 'get_or_compute_bloom_filter'. The former only
> > looks up a Bloom filter (and does not compute one if it's missing,
> > thus dropping the 'compute_if_not_present' flag). The latter does
> > compute missing Bloom filters, with an additional parameter to store
> > whether or not it needed to do so.
> >
> > This simplifies many call-sites, since the majority of existing callers
> > to 'get_bloom_filter' do not want missing Bloom filters to be computed
> > (so they can drop the parameter entirely and use the simpler version of
> > the function).
>
> > +struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
> > +						 struct commit *c,
> > +						 int compute_if_not_present,
> > +						 int *computed)
>
> Could we further simplify this by letting "computed" be the indicator
> for whether we should compute the filter? If "computed" is NULL, then
> we won't compute it directly. This allows us to reduce the "1, NULL)"
> to "NULL)" in these callers:

I like what you're getting at--that is, that we shouldn't make calling
this more complicated than necessary, and right now lots of callers
always pass "1, NULL)" as the last two arguments--but I'm not sure that
I like this suggestion.

I could imagine a future caller would want to compute the Bloom filters
if missing, but not care about whether or not they were computed from
scratch. In that case, they'd need a dummy variable. Not the worst thing
in the world, but I think it's less clear.

By the way, I think that this suggestion only helps for "0, NULL" into
just "NULL", not "1, NULL" (which requires a dummy variable with your
suggestion).

>
> > +			struct bloom_filter *filter = get_or_compute_bloom_filter(ctx->r, c, 1, NULL);
>
> > +	filter = get_or_compute_bloom_filter(the_repository, c, 1, NULL);
>
> Thanks,
> -Stolee

Thanks,
Taylor