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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 25, 2025, 17:43 UTC
Message-ID: <xmqqtt43u36t.fsf@gitster.g>
In-Reply-To: <20250625125541.3048632-2-502024330056@smail.nju.edu.cn>

Lidong Yan <yldhome2d2@gmail.com> writes:

Show 5 quoted lines

> +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];
	};

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].

Show 8 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.

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;
> +}

> +struct bloom_key *bloom_keyvec_at(const struct bloom_keyvec *v, size_t i)
> +{

Ditto about abusing the name 'i'.

Show 6 quoted lines

> +			return ret;
> +	}
> +
> +	return 1;
> +}
> \ No newline at end of file

Tell your editor to be more careful, perhaps?