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
