Re: [PATCH v3 1/2] bloom: replace struct bloom_key * with struct bloom_keyvec

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 2, 2025, 18:28 UTC
Message-ID: <xmqqy0t6curr.fsf@gitster.g>
In-Reply-To: <aGVLZ9VUf2M1sWhL@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 10 quoted lines

>> +static inline void fill_bloom_keyvec_key(const char *data, size_t len,
>> +					 struct bloom_keyvec *vec, size_t nr,
>> +					 const struct bloom_filter_settings *settings)
>> +{
>> +	assert(nr < vec->count);
>> +	fill_bloom_key(data, len, &vec->key[nr], settings);
>> +}
>> +
>
> Similarly, this should probably be called `bloom_keyvec_fill_key()`.

If so, a preliminary clean-up patch in front of the series is in order, as the <bloom.h> header file, without these patches, already has the follwoing external API functions and structures declared, that do not follow your naming rules at all (I have removed the ones that begin with "bloom_" from the below):

    int load_bloom_filter_from_graph()
    uint32_t murmur3_seeded_v2();
    void fill_bloom_key();
    void clear_bloom_key(s);
    void add_key_to_filter();
    void init_bloom_filters(void);
    void deinit_bloom_filters(void);
    struct bloom_filter *get_or_compute_bloom_filter();
    struct bloom_filter *get_bloom_filter();

It is very dubious that murmur3_seeded_v2() is exposed (nobody would know it is for Bloom filter subsystem from that name); as far as I can tell, it is only needed for t/helper testing, and makes me wonder if we can come up with a better division between the production code and t/helper/ code around there.

Thanks.