diff mbox series

[v2,1/3] git-compat-util: introduce BUG_IF_NOT() macro

Message ID 109060ccb8665c73aa0c4f73e3cbbddcd135bde4.1742107322.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Add a static analysis job to prevent assertions with side effects | expand

Commit Message

Elijah Newren March 16, 2025, 6:42 a.m. UTC 
From: Elijah Newren <newren@gmail.com>

Create a BUG_IF_NOT() macro which is similar to assert(), but will not be
compiled out when NDEBUG is defined, and is thus safe to use even if its
argument has side-effects.

We will use this new macro in a subsequent commit to convert a few
existing assert() invocations to BUG_IF_NOT().  In particular, we'll
convert the handful of invocations which cannot be proven to be free of
side effects with a simple compiler/linker hack.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 git-compat-util.h | 1 +
 1 file changed, 1 insertion(+)

Comments

Junio C Hamano March 17, 2025, 10:33 p.m. UTC | #1 
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Elijah Newren <newren@gmail.com>
>
> Create a BUG_IF_NOT() macro which is similar to assert(), but will not be
> compiled out when NDEBUG is defined, and is thus safe to use even if its
> argument has side-effects.

If this is meant to be "similar to" assert, let's not call it
BUG_IF_NOT().  The point of BUG() is that the developer can mark the
problem with something more than just a conditional, and it feels
funny to call a facility that lacks that central feature with a name
with BUG in it.

ASSERT(), safer_assert(), safe_assert(), sane_assert()?  

The last one is in line with safe_istest() that is used on
sane_ctype[] and sane_qsort(), with the intention to allow
developers to write right code more easily than using the plain
vanilla C.

Thanks.
Taylor Blau March 17, 2025, 10:36 p.m. UTC | #2 
On Mon, Mar 17, 2025 at 03:33:50PM -0700, Junio C Hamano wrote:
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > Create a BUG_IF_NOT() macro which is similar to assert(), but will not be
> > compiled out when NDEBUG is defined, and is thus safe to use even if its
> > argument has side-effects.
>
> If this is meant to be "similar to" assert, let's not call it
> BUG_IF_NOT().  The point of BUG() is that the developer can mark the
> problem with something more than just a conditional, and it feels
> funny to call a facility that lacks that central feature with a name
> with BUG in it.
>
> ASSERT(), safer_assert(), safe_assert(), sane_assert()?
>
> The last one is in line with safe_istest() that is used on
> sane_ctype[] and sane_qsort(), with the intention to allow
> developers to write right code more easily than using the plain
> vanilla C.

For my $.02, I prefer ASSERT() to the other options. It's clear, but
indicates that it's a macro and thus not the same as assert(3). But I
don't have a strong opinion here.

Thanks,
Taylor
diff mbox series

Patch

diff --git a/git-compat-util.h b/git-compat-util.h
index e123288e8f1..c3415ad7e0a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1460,6 +1460,7 @@  extern int bug_called_must_BUG;
 __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__)
+#define BUG_IF_NOT(a) if (!(a)) BUG("Assertion `" #a "' failed.")
 __attribute__((format (printf, 3, 4)))
 void bug_fl(const char *file, int line, const char *fmt, ...);
 #define bug(...) bug_fl(__FILE__, __LINE__, __VA_ARGS__)