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

Junio C Hamano [off-list ref] writes

Lidong Yan [off-list ref] writes:

quoted

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

quoted

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

If you make keyvec an fixed flex-array, the below would become
unnecessary (and the check for NULL-ness of .keys[] array).

quoted

+
+ FREE_AND_NULL(v->keys);
+ v->nr = 0;
+}

Another benefit to use flex-array.

quoted

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

Ditto about abusing the name 'i'.

quoted

+ 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