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@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. 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
