Re: [PATCH 1/5] usage.c: add a non-fatal bug() function to go with BUG()

From: Junio C Hamano <gitster@pobox.com>
Date: May 25, 2022, 20:55 UTC
Message-ID: <xmqqpmk15o46.fsf@gitster.g>
In-Reply-To: <patch-1.5-faa1c708a79-20220521T170939Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Any caller to bug() should follow up such calls with BUG_if_bug(),
> which will BUG() out (i.e. abort()) if there were any preceding calls
> to bug(). As the tests and documentation here show we'll catch missing
> BUG_if_bug() invocations in our exit() wrapper.

...

> +- `bug` (lower-case, not `BUG`) is supposed to be used like `BUG` but
> +  prints a "BUG" message instead of calling `abort()`. We then expect
> +  `BUG_if_bug()` to be called to `abort()` if there were any calls to
> +  `bug()`. If there weren't any a call to `BUG_if_bug()` is a NOOP.

OK. So the expected pattern would be a series of calls to bug(), each guarded by its own condition, concluded by a call to BUG_if_bug()

	if (condition1)
		bug(...);
	if (condition2)
		bug(...);
	...
	BUG_if_bug();

and when none of the guard conditions fired, BUG_if_bug() will become a no-op.

> +/* usage.c: if bug() is called we should have a BUG_if_bug() afterwards */
> +extern int bug_called_must_BUG;

I am not sure about the name, but in essense, each call to bug() ensures that this becomes true, so BUG_if_bug() can use it to see if it should abort(), right?

Show 12 quoted lines

>  __attribute__((format (printf, 3, 4))) NORETURN
>  void BUG_fl(const char *file, int line, const char *fmt, ...);
>  #define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)
> +__attribute__((format (printf, 3, 4)))
> +void bug_fl(const char *file, int line, const char *fmt, ...);
> +#define bug(...) bug_fl(__FILE__, __LINE__, __VA_ARGS__)
> +#define BUG_if_bug() do { \
> +	if (bug_called_must_BUG) { \
> +		bug_called_must_BUG = 0; \
> +		BUG_fl(__FILE__, __LINE__, "see bug() output above"); \
> +	} \
> +} while (0)

I have a feeling that "see bug() output above" should come from the caller of BUG_if_bug(). These bug() calls that are grouped together must have a shared overall theme, which may want to be stated in that final message.

Other than these two small points, this does not look too bad ;-)

Thanks.