Siddharth Singh [off-list ref] writes:

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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