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

From: Lidong Yan <502024330056@smail.nju.edu.cn>
Date: Jun 26, 2025, 03:44 UTC
Message-ID: <4E0C5F0C-16BA-4B7B-BC5B-632CFEA871E3@smail.nju.edu.cn>
In-Reply-To: <xmqqtt43u36t.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes

Show 17 quoted lines

> 
> Lidong Yan <yldhome2d2@gmail.com> writes:
> 
>> +void bloom_keyvec_init(struct bloom_keyvec *v, size_t initial_size)
>> +{
>> + ALLOC_ARRAY(v->keys, initial_size);
>> + v->nr = initial_size;
>> +}
> 
> Hmph, does this ever grow once initialized?  When I outlined the
> solution in the earlier discussion, I was wondering a structure that
> looks more like
> 
> struct bloom_keyvec {
> size_t count;
> struct bloom_key key[FLEX_ARRAY];
> };

I understand. If we don't need to resize the array dynamically, we should allocate the array elements and the array length together to gain benefits such as spatial locality.

> Also when your primary use of an array is to use one element at a
> time (as opposed to the entire array as a single "collection"), name
> it singular, so that key[4] is more naturally understood as "4-th
> key", not keys[4].

Got it.

Show 19 quoted lines

> 
>> +void bloom_keyvec_clear(struct bloom_keyvec *v)
>> +{
>> + size_t i;
>> + if (!v->keys)
>> + return;
>> +
>> + for (i = 0; i < v->nr; i++)
>> + clear_bloom_key(&v->keys[i]);
> 
> By doing
> 
> for (size_t nr; nr < v->nr; nr++)
> 
> you can
> 
> - lose the separate local variable definition at the beginning;
> - avoid confusing "i", which hints to be an "int", to be of type "size_t"
> - limit the scope of "nr" a bit tigher.

Got it.

Show 8 quoted lines

> 
> If you make keyvec an fixed flex-array, the below would become
> unnecessary (and the check for NULL-ness of .keys[] array).
> 
>> +
>> + FREE_AND_NULL(v->keys);
>> + v->nr = 0;
>> +}

Another benefit to use flex-array.

Show 14 quoted lines

> 
>> +struct bloom_key *bloom_keyvec_at(const struct bloom_keyvec *v, size_t i)
>> +{
> 
> Ditto about abusing the name 'i'.
> 
>> + return ret;
>> + }
>> +
>> + return 1;
>> +}
>> \ No newline at end of file
> 
> Tell your editor to be more careful, perhaps?

Seems git clang-format doesn’t add newline for me, I will try .editconfig next time.

Thanks, Lidong