Re: [PATCH v4 4/7] bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 24, 2023, 06:34 UTC
- Message-ID
- <ZTdlgEaRLpdSjB1n@tanuki>
- In-Reply-To
- <20231023185842.GE1537181@coredump.intra.peff.net>
On Mon, Oct 23, 2023 at 02:58:42PM -0400, Jeff King wrote:
Show 18 quoted lines
> On Mon, Oct 23, 2023 at 11:19:13AM +0200, Patrick Steinhardt wrote: > > > > + case SOURCE_INCORE: > > > + assert(source->read <= source->size); > > > > Is there any guideline around when to use `assert()` vs `BUG()`? I think > > that this assertion here is quite critical, because when it does not > > hold we can end up performing out-of-bounds reads and writes. But as > > asserts are typically missing in non-debug builds, this safeguard would > > not do anything for our end users, right? > > I don't think we have a written guideline. My philosophy is: always use > BUG(), because you will never be surprised that the assertion was not > compiled in (and I think compiling without assertions is almost > certainly premature optimization, especially given the way we tend to > use them). > > -Peff
I'm inclined to agree with your philosophy. Makes me wonder whether we should write a Coccinelle rule to catch this. But a quick-and-dirty grep in our codebase shows that such a rule would cause quite a lot of churn:
$ git grep BUG\( | wc -l 677 $ git grep assert\( | wc -l 549
Probably not worth it.
Patrick