klib icon indicating copy to clipboard operation
klib copied to clipboard

khash example in gh-pages documentation uses non-existing variable

Open rofl0r opened this issue 5 years ago • 7 comments

the example uses if(!ret) even though ret isn't declared. i assume k is meant.

rofl0r avatar Jun 21 '19 23:06 rofl0r

I had the same problem. Could not figure out the fix due to the huge macro beyond my experience.
Is there a fix of that please? Thanks a lot.

yifangt avatar Dec 05 '19 18:12 yifangt

It should be absent in example 1. If you look at the documentation for kh_put():

/*! @function
  @abstract     Insert a key to the hash table.
  @param  name  Name of the hash table [symbol]
  @param  h     Pointer to the hash table [khash_t(name)*]
  @param  k     Key [type of keys]
  @param  r     Extra return code: -1 if the operation failed;
                0 if the key is present in the hash table;
                1 if the bucket is empty (never used); 2 if the element in
				the bucket has been deleted [int*]
  @return       Iterator to the inserted element [khint_t]
 */
#define kh_put(name, h, k, r) kh_put_##name(h, k, r)

absent == 0 tells you that the key was already present in the table.

Honestly though, line 8 should be deleted. Calling kh_del() would invalidate the k iterator so calling kh_value() with it would then be invalid.

selavy avatar Dec 05 '19 19:12 selavy

@selavy Thanks! To follow the original logic of the code, the variable ret should be absent, i.e. if (!absent) right?

yifangt avatar Dec 05 '19 23:12 yifangt

@yifangt

The original logic of the code doesn't make sense. Hopefully this code snippet is explains why:

#include <assert.h>
#include <stdbool.h>
#include "khash.h"

enum {
    ERROR           = -1,
    ELEMENT_PRESENT = 0,
    EMPTY           = 1,
    TOMBSTONE       = 2,
};

KHASH_MAP_INIT_INT(m32, char)
int main(int argc, char **argv)
{
    int absent;
    int is_missing;
    khint_t k;

    // initialize the table
    khash_t(m32) *h = kh_init(m32);

    // insert a key of 5 (N.B. the value is NOT set)
    k = kh_put(m32, h, 5, &absent);
    assert(kh_size(h) == 1);
    assert(absent != ELEMENT_PRESENT);
    assert(absent == EMPTY);

    // delete the key we just inserted
    kh_del(m32, h, k);
    assert(kh_size(h) == 0);
    assert(kh_exist(h, k) == false); // iterator is invalid now
    assert(kh_get(m32, h, 5) == kh_end(h));

    // because kh_exist(h, k) == false, the line below is a bug:
    // kh_value(h, k) = 10;

    // re-insert a key of 5 (N.B. the value is still not set)
    k = kh_put(m32, h, 5, &absent);
    assert(kh_size(h) == 1);
    assert(absent == TOMBSTONE); // tombstone because we deleted the key
    assert(kh_get(m32, h, 5) != kh_end(h));

    // now set the value
    kh_value(h, k) = 10;
    assert(kh_value(h, kh_get(m32, h, 5)) == 10);

    return 0;
}

selavy avatar Dec 06 '19 02:12 selavy

Thanks a lot! I need more time to understand the huge macro, especially the logic wrapped inside.

yifangt avatar Dec 10 '19 16:12 yifangt

I would recommend reading through the documentation that is in the file: https://github.com/attractivechaos/klib/blob/f719aad5fa273424fab4b0d884d68375b7cc2520/khash.h#L428-L549

selavy avatar Dec 10 '19 22:12 selavy

I would recommend reading through the documentation that is in the file:

i would recommend opening a PR fixing the docs, but then it looks like this repo is unmaintained anyways.

rofl0r avatar Dec 11 '19 05:12 rofl0r