Re: [PATCH 4/4] line-log: convert an assertion to a full BUG() call

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 5, 2018, 10:42 UTC
Message-ID: <CAPig+cRrC2mf1uuQ1C4Ue4OMZQbgcxXbJ9AXs0y6RSnUrcm7Dg@mail.gmail.com>
In-Reply-To: <faf35214f0f339b792a30a3bd013056217d9a2c1.1533421101.git.gitgitgadget@gmail.com>

On Sat, Aug 4, 2018 at 6:18 PM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 11 quoted lines

> The assertion in question really indicates a bug, when triggered, so we
> might just as well use the sanctioned method to report it.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/line-log.c b/line-log.c
> @@ -72,7 +72,9 @@ void range_set_append(struct range_set *rs, long a, long b)
> -       assert(rs->nr == 0 || rs->ranges[rs->nr-1].end <= a);
> +       if (rs->nr > 0 && rs->ranges[rs->nr-1].end > a)
> +               BUG("append %ld-%ld, after %ld-%ld?!?", a, b,
> +                   rs->ranges[rs->nr-1].start, rs->ranges[rs->nr-1].end);

Although this appears to be a faithful translation of the assert() to BUG(), as mentioned by Andrei in his review of 3/4, the existing assert() seems to have an off-by-1 error, which means that the "> a" here really ought to be ">= a".

Given that this file is full of assert()'s, it doesn't necessarily make sense to convert only this one, so perhaps the patch should be dropped (since I'm guessing you don't want to convert the rest of them).