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

[Michal Hocko <mhocko@suse.com>]

On Wed 06-01-21 12:10:30, Hugh Dickins wrote:
> On Wed, 6 Jan 2021, Andrew Morton wrote:
> > On Tue, 5 Jan 2021 20:28:27 -0800 (PST) Hugh Dickins <hughd@google.com> 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.

I agree with Hugh. BUG_ON on something that is not a trivial predicate
makes the code slightly harder to follow.

I also do agree that accomodating the coding style to the existing code
is better as well because the resulting code is more compact.

In general I consider code transformations like this without a higher
goal that is stated explicitly a pointless churn which doesn't bring
much while it consumes a very scarce review bandwidth. Even when those
look trivial there is always a room to introduce silent breakage.
Be it a checkpatch or coccinelle the change shouldn't be based solely on
the script complains. Really, what is the point of changing an existing
if (cond) BUG into BUG_ON? Fewer lines? Taste? Code consistency?
-- 
Michal Hocko
SUSE Labs