[Freeipa-devel] [PATCH] add dynamic hash table data structure implementation (with review modifications)

[Martin Nagy]

Hi,
I didn't really review the code, but I took a quick peek out of
curiosity and noticed few bits.

John Dennis wrote:
> From 5788dcd44bafd76d5d8a843d30c2178ce34397ce Mon Sep 17 00:00:00 2001
> From: John Dennis <jdennis at redhat.com>
> Date: Thu, 16 Apr 2009 17:48:13 -0400
> Subject: [PATCH] add dynamic hash table data structure implementation
> 
> Apply suggested fixes by Simo after code review
> * return statements no longer use () unless it's an expression
You actually don't need () even if it's a more complicated expression.

BTW, you don't need to include comments of what changes you did since
your last patch in the commit message itself.

> +#define SEGMENT_SIZE            32
> +#define SEGMENT_SIZE_SHIFT      5	/* log2(SEGMENT_SIZE) */
> +#define DIRECTORY_SIZE          32
> +#define DIRECTORY_SIZE_SHIFT    5	/* log2(DIRECTORY_SIZE) */
> +#define PRIME_1                 37
> +#define PRIME_2                 1048583
> +#define DEFAULT_MAX_LOAD_FACTOR 5
> +
> + /*
> +  * Fast arithmetic, relying on powers of 2, and on pre-processor
> +  * concatenation property
> +  */
> +
> +#define MUL(x,y)               ((x) << (y##_SHIFT))
> +#define DIV(x,y)               ((x) >> (y##_SHIFT))
> +#define MOD(x,y)               ((x) & ((y)-1))
No need for these, please leave this on the compiler. GCC will turn it
into shifts even if you don't optimize. Also, you have to be careful
with signed integers. Shifting a signed integer to the right is not the
same as dividing it.

> +struct hash_table_str {
> +    long   p;				/* Next bucket to be split */
> +    long   maxp;			/* upper bound on p during expansion */
> +    long   entry_count;			/* current # entries */
> +    long   bucket_count;		/* current # buckets */
> +    long   segment_count;		/* current # segments */
> +    long   min_load_factor;
> +    long   max_load_factor;
> +    hash_delete_callback delete_callback;
> +    segment_t *directory[DIRECTORY_SIZE];
> +#ifdef HASH_STATISTICS
> +    long   hash_accesses, hash_collisions, table_expansions, table_contractions;
> +#endif
> +
> +};
Would it be possible to make some of these members unsigned (see
my previous comment)? Also, you're using tabs here (also on couple of
other places).

Martin