Siddharth Singh <siddhartth@xxxxxxxxxx> writes:
>> > +int initialized_hashmap_pointer_not_null() {
>> > + kh_str_t *hashmap = kh_init_str();
>> > +
>> > + if(hashmap != NULL){
>> > + free(hashmap);
>> > + return 1;
>> > + }
>> > + return 0;
>> > +}
>>
>> I don't think this is a useful test. It would be extremely weird for the
>> return value to be NULL in regular usage.
>
> While it is indeed uncommon for the return value of kh_init_str() to be null in
> regular usage, there could be certain cases where the hashmap pointer might be
> null. One possible scenario is if there is an error during memory allocation for
> the hashmap. This can happen if the system runs out of memory or if there are
> other memory-related issues. In such cases, the kh_init_str() function might
> return a null pointer to indicate the failure to allocate memory for the hashmap.
Ah, yes, that is what I expected. By "regular usage", I meant the case
where the allocation succeeds. Nevertheless, I don't think this test
demonstrates or asserts anything that isn't easily observed in other
tests, though I am open to be proven wrong.
>> > +int put_value_into_hashmap_once_succeeds() {
>> > + int ret, value = 14;
>> > + khint_t pos;
>> > + kh_str_t *hashmap = kh_init_str();
>> > + pos = kh_put_str(hashmap, "test_key", &ret);
>> > + if (!ret)
>> > + return 0;
>> > + kh_key(hashmap, pos) = xstrdup("test_key");
>> > + kh_value(hashmap, pos) = &value;
>> > + return *kh_value(hashmap, pos) == value;
>> > +}
>> Also, do we have to kh_put_str("some_key") and then immediately set it
>> again with kh_key(pos)? That seems odd, and I don't see other callers
>> doing that all the time.
>
> Regarding the usage of kh_value(), I saw this part of code[1] This suggests
> that it may be the recommended approach in this context.
What 'context' were you thinking of when you say "recommended approach
in this context"? (Did you mean kh_put_str() vs kh_put_oid_*()?) I
didn't see such an approach in most kh_put_*() call sites. I couldn't
find the link you're referencing, but I assume it is the following lines
in delta-islands.c:
int hash_ret;
khiter_t pos = kh_put_str(remote_islands, island_name, &hash_ret);
if (hash_ret) {
kh_key(remote_islands, pos) = xstrdup(island_name);
kh_value(remote_islands, pos) = xcalloc(1, sizeof(struct remote_island));
}
In which case, the use case (as I understand it at least) seems quite
different:
- When kh_put_str() 'returns' hash_ret = 0, it means there's already a
matching entry and we should do nothing.
- Otherwise, kh_put_str() actually creates a new matching entry, and now
we want to allocate memory to store the entry, so we overwrite the key
with "kh_key() = xstrdup()". We could have avoided this by doing
something like:
char *key = xstrdup(island_name)
khiter_t pos = kh_put_str(remote_islands, key, &hash_ret);
if (hash_ret) {
kh_value(remote_islands, pos) = xcalloc(1, sizeof(struct remote_island));
} else {
free(key);
}
but allocations are expensive, so we should avoid allocating before we
know it's needed.
>> > +int put_same_value_into_hashmap_twice_fails() {
>> > + int first_return_value, second_return_value, value = 14;
>> > + khint_t pos;
>> > + kh_str_t *hashmap = kh_init_str();
>> > + pos = kh_put_str(hashmap, "test_key", &first_return_value);
>> > + if (!first_return_value)
>> > + return 0;
>> > + kh_key(hashmap, pos) = xstrdup("test_key");
>> > + kh_value(hashmap, pos) = &value;
>> > + kh_put_str(hashmap, "test_key", &second_return_value);
>> > + return !second_return_value;
>> > +}
>>
>> I don't see how this is different from the previous test that tested for
>> collisions.
>
> I also attempt to assign a value to the hashmap here, but this will not be
> successful due to a collision. The intent of this test was different, however
Hm, what was the different intent? Is it to test this slightly different
case where the value is assigned?
My reasoning is:
- Would assigning a value result in different behavior?
- Would a reasonable user expect that it would result in different
behavior?
If the answer to both of those is 'no' (which I believe it is), then it
is not worth testing.
