Re: [PATCH] use DIV_ROUND_UP

From: Jeff King <peff@peff.net>
Date: Jul 10, 2017, 07:27 UTC
Message-ID: <20170710072701.cqzztww6mrqr7sli@sigill.intra.peff.net>
In-Reply-To: <4d2c274a-f6cb-6ea5-304f-51a3152cc436@web.de>

On Sat, Jul 08, 2017 at 12:35:35PM +0200, René Scharfe wrote:

> Convert code that divides and rounds up to use DIV_ROUND_UP to make the
> intent clearer and reduce the number of magic constants.

Sounds like a good idea.

> -	auto_threshold = (gc_auto_threshold + 255) / 256;
> +	auto_threshold = DIV_ROUND_UP(gc_auto_threshold, 256);

DIV_ROUND_UP(n,d) is defined as (n+d-1)/d. So this is clearly a mechanical conversion and thus correct. And most cases are like this, but...

Show 12 quoted lines

> diff --git a/ewah/ewah_bitmap.c b/ewah/ewah_bitmap.c
> index 2dc9c82ecf..06c479f70a 100644
> --- a/ewah/ewah_bitmap.c
> +++ b/ewah/ewah_bitmap.c
> @@ -210,8 +210,8 @@ size_t ewah_add(struct ewah_bitmap *self, eword_t word)
>  void ewah_set(struct ewah_bitmap *self, size_t i)
>  {
>  	const size_t dist =
> -		(i + BITS_IN_EWORD) / BITS_IN_EWORD -
> -		(self->bit_size + BITS_IN_EWORD - 1) / BITS_IN_EWORD;
> +		DIV_ROUND_UP(i + 1, BITS_IN_EWORD) -
> +		DIV_ROUND_UP(self->bit_size, BITS_IN_EWORD);

...this first one is a bit trickier. Our "n" in the first one is now "i+1". But that's because the original was implicitly canceling the "-1" and "+1" terms.

So I think it's a true mechanical conversion, but I have to admit the original is confusing. Without digging I suspect it's correct, though, just because a simple bug here would mean that our ewah bitmaps totally don't work. So it's probably not worth spending time on.

> [...]

And all the others are straight-forward and obviously correct. So this patch looks good to me.

-Peff