Re: [PATCH] mm/mmap: replace if (cond) BUG() with BUG_ON()

[Hugh Dickins]

On Wed, 6 Jan 2021, Andrew Morton wrote:
> On Tue, 5 Jan 2021 20:28:27 -0800 (PST) Hugh Dickins <hughd@xxxxxxxxxx> wrote:
> 
> > Alex, please consider why the authors of these lines (whom you
> > did not Cc) chose to write them without BUG_ON(): it has always
> > been preferred practice to use BUG_ON() on predicates, but not on
> > functionally effective statements (sorry, I've forgotten the proper
> > term: I'd say statements with side-effects, but here they are not
> > just side-effects: they are their main purpose).
> > 
> > We prefer not to hide those away inside BUG macros
> 
> Should we change that?  I find BUG_ON(something_which_shouldnt_fail())
> to be quite natural and readable.

Fair enough.  Whereas my mind tends to filter out the BUG lines when
skimming code, knowing they can be skipped, not needing that effort
to pull out what's inside them.

Perhaps I'm a relic and everyone else is with you: I can only offer
my own preference, which until now was supported by kernel practice.

> 
> As are things like the existing
> 
> BUG_ON(mmap_read_trylock(mm));
> BUG_ON(wb_domain_init(&global_wb_domain, GFP_KERNEL));
> 
> etc.

People say "the exception proves the rule".  Perhaps we should invite a
shower of patches to change those?  (I'd prefer not, I'm no fan of churn.)

> 
> No strong opinion here, but is current mostly-practice really
> useful?

You've seen my vote.  Now let the games begin!

Hugh