On 3/31/25 01:01, Rahila Syed wrote:
> ...
>
>
> > I will improve the comment in the next version.
> >
>
> OK. Do we even need to pass nelem_alloc to hash_get_init_size? It's not
> really used except for this bit:
>
> + if (init_size > nelem_alloc)
> + element_alloc = false;
>
> Can't we determine before calling the function, to make it a bit less
> confusing?
>
>
> Yes, we could determine whether the pre-allocated elements are zero before
> calling the function, I have fixed it accordingly in the attached 0001
> patch.
> Now, there's no need to pass `nelem_alloc` as a parameter. Instead, I've
> passed this information as a boolean variable-initial_elems. If it is
> false,
> no elements are pre-allocated.
>
> Please find attached the v7-series, which incorporates your review patches
> and addresses a few remaining comments.
>
I think it's almost committable. Attached is v8 with some minor review
adjustments, and updated commit messages. Please read through those and
feel free to suggest changes.
I still found the hash_get_init_size() comment unclear, and it also
referenced init_size, which is no longer relevant. I improved the
comment a bit (I find it useful to mimic comments of nearby functions,
so I did that too here). The "initial_elems" name was a bit confusing,
as it seemed to suggest "number of elements", but it's a simple flag. So
I renamed it to "prealloc", which seems clearer to me. I also tweaked
(reordered/reformatted) the conditions a bit.
For the other patch, I realized we can simply MemSet() the whole chunk,
instead of resetting the individual parts.
regards
--
Tomas Vondra
