Re: [PATCH v2] khash: clarify that allocations never fail

From: Jeff King <peff@peff.net>
Date: Jul 4, 2021, 09:05 UTC
Message-ID: <YOF55hIKhRrwrZah@coredump.intra.peff.net>
In-Reply-To: <3778fb28-ed19-e90e-216a-d29d72305155@web.de>

On Sat, Jul 03, 2021 at 02:57:30PM +0200, René Scharfe wrote:

Show 6 quoted lines

> We use our standard allocation functions and macros (xcalloc,
> ALLOC_ARRAY, REALLOC_ARRAY) in our version of khash.h.  They terminate
> the program on error instead, so code that's using them doesn't have to
> handle allocation failures.  Make this behavior explicit by turning
> kh_resize_ into a void function and removing the related unreachable
> error handling code.

Thanks, this looks good to me.

Show 14 quoted lines

>  	SCOPE khint_t kh_put_##name(kh_##name##_t *h, khkey_t key, int *ret) \
>  	{																	\
>  		khint_t x;														\
>  		if (h->n_occupied >= h->upper_bound) { /* update the hash table */ \
>  			if (h->n_buckets > (h->size<<1)) {							\
> -				if (kh_resize_##name(h, h->n_buckets - 1) < 0) { /* clear "deleted" elements */ \
> -					*ret = -1; return h->n_buckets;						\
> -				}														\
> -			} else if (kh_resize_##name(h, h->n_buckets + 1) < 0) { /* expand the hash table */ \
> -				*ret = -1; return h->n_buckets;							\
> +				kh_resize_##name(h, h->n_buckets - 1); /* clear "deleted" elements */ \
> +			} else { \
> +				kh_resize_##name(h, h->n_buckets + 1); /* expand the hash table */ \
>  			}															\

I had to read this over again carefully, because the second "else-if" makes the conversion less obvious. I think the original would have been easier to read as:

  if (something) {
    if (do_option_one())
        ...error...
  } else {
    if (do_option_two())
        ...error...
  }

instead of:

  if (something) {
    if (do_option_one())
        ...error...
  } else if (do_option_two())
        ...error...
  }

All of which is to say the conversion here is correct, and I think as a bonus the resulting code is easier to follow. ;)

-Peff