Re: [PATCH 1/1] irq-gic: use BUG_ON instead of if()/BUG

[Thomas Gleixner]

On Fri, 19 Jun 2015, Maninder Singh wrote:
> Hi Thomas,
> 
> >>  {
> >> -	if (gic_nr >= MAX_GIC_NR)
> >> -		BUG();
> >> +	BUG_ON(gic_nr >= MAX_GIC_NR);
> >>  	if (irq_set_handler_data(irq, &gic_data[gic_nr]) != 0)
> >>  		BUG();
> >
> >So this patch was clearly done just by running a script and not sanity
> >checked afterwards. Otherwise the next if() BUG(); construct would
> >have been fixed as well.
> 
> Yes semantic patch did the changes to use preferred APIs,
> And it also changed this BUG_ON(irq_set_handler_data(irq, &gic_data[gic_nr]) != 0)
> But we have to take care that  condition has no side effects i.e.
> 
> if()/BUG conversion to BUG_ON must be avoided when there's side effect
> in condition. The reason being BUG_ON won't execute the condition when CONFIG_BUG
> is not defined As suggested by Julia Lawall
> 
> Thats why did not take that change  --> (BUG_ON(irq_set_handler_data(irq, &gic_data[gic_nr]) != 0))

Fair enough. But you should mention that in the changelog.
 
> >Further, while we are at that. It would be even more useful to analyze
> >whether the BUG_ON() is needed in the first place or at least could be
> >made conditional on some debug option.
> >
> >But that's not done by the script either, right?
> 
> Yes coccinelle semantic patches did not do that changes.
> we have to choose whether to make BUG_ON conditional on some debug options.

Right, and that's what I'm asking for. IOW, instead of blindly running
scripts at least ask the question whether this stuff needs to be there
unconditionally....

Such information can be put into the changelog and helps the reviewers
to distinguish thoughtful people from script bots.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/