Messages in this thread | | | Date | Fri, 19 Jun 2015 09:51:36 +0000 (GMT) | From | Maninder Singh <> | Subject | Re: [PATCH 1/1] irq-gic: use BUG_ON instead of if()/BUG |
| |
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))
>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.
Thanks, Maninder
| |