attractivechaos / klib

A standalone and lightweight C library
http://attractivechaos.github.io/klib/
MIT License
4.18k stars 556 forks source link

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

Open rofl0r opened 5 years ago

rofl0r commented 5 years ago

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

yifangt commented 4 years ago

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.

selavy commented 4 years ago

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.

yifangt commented 4 years ago

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

selavy commented 4 years ago

@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;
}
yifangt commented 4 years ago

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

selavy commented 4 years ago

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

rofl0r commented 4 years ago

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.