Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

From: Jeff King <peff@peff.net>
Date: Nov 22, 2017, 23:58 UTC
Message-ID: <20171122235851.GE8577@sigill>
In-Reply-To: <20171122234532.GI11671@aiede.mtv.corp.google.com>

On Wed, Nov 22, 2017 at 03:45:32PM -0800, Jonathan Nieder wrote:

Show 6 quoted lines

> Jeff King wrote:
> 
> > Yes. I'd be fine having a single-argument BUG_ON() like that. But then,
> > I'm not sure what it's buying us over assert().
> 
> It lets you build with NDEBUG.

But why do you want to build with NDEBUG if nothing uses assert()? ;)

I'm being a little glib, but I think there really is a chicken-and-egg question here. Why are people building with NDEBUG if they don't want to disable asserts? Do they do it out of habit? A misguided sense of performance optimization? Does it do something else I'm not aware of?

> It also goes through our own die()
> handler, which means that e.g. the error message gets propagated over
> remote transports.

BUG() doesn't go through our die handler. It hits vreportf(), which does do some minor munging, but it always goes to stderr. Error message propagation over the remote protocol happens either:

  1. From a parent process muxing our stderr onto the sideband.

  2. Via special wrappers like receive-pack's rp_error().

The only program I know that sets a custom die handler to change the reporting is git-http-backend (and there it only applies to die("BUG"), not BUG(), so with the latter you'd get a hard hangup instead of an attempt at an http 500).

Show 6 quoted lines

> Please please please, don't rely on side-effects from assert().  They
> will cause me to run into pain over time.  This issue alone might be
> worth banning use of assert() in the codebase, if we can't trust
> reviewers to catch problematic examples (fortunately reviewers have
> been pretty good about that so far, but banning it would free up their
> attention to focus on other aspects of a patch).

Yes, obviously side effects in an assert() are horrible. But I haven't noticed that being a problem in our code base.

For the record, I'm totally fine with banning assert() in favor of a custom equivalent. I just don't think we've seen any real problems with assert in our codebase so far.

-Peff