Messages in this thread | | | Date | Sun, 21 Mar 2010 15:19:58 +0100 (CET) | From | Julia Lawall <> | Subject | Re: [PATCH 06/12] genericirq: make irq_chip related function to take desc |
| |
On Sun, 21 Mar 2010, Eric W. Biederman wrote:
> Julia Lawall <julia@diku.dk> writes: > > > I also worked on this, but only sent it to Thomas and Yinghai. Onthe > > other hand, I mostly like your solution better, because it has the > > unintended side-effect of getting rid of some blank spaces after {s. > > Wow I hadn't noticed that {s removal. That is an old coding style > violation on alpha. There was an intentional effect of not breaking > up { }'s. But I hadn't realized I was also fixing whitespace.
I think it is because you added a local declaration of irq and then removed it when it was not necessary. So then coccinelle was motivated to remove the space after the {.
> > My rule was also more complicated in that it also searches for conditions > > in which it is not sure to be doing the right thing. I will send those in > > another message. > > > >> @ DECL @ > >> struct irq_chip CHIP; > >> identifier METHOD; > >> identifier METHOD_NAME; > >> @@ > >> CHIP.METHOD_NAME = METHOD; > >> > >> @ @ > >> identifier DECL.METHOD; > >> identifier IRQ; > >> @@ > >> METHOD( > >> - unsigned int IRQ > >> + struct irq_desc *unused > >> , ...) { > >> } > > > > I didn't think of making a special rule for this. It could consider any > > case where the body is ... when != IRQ > > Nice addition. > > >> @ @ > >> identifier DECL.METHOD; > >> identifier IRQ; > >> identifier DESC; > >> @@ > >> METHOD( > >> - unsigned int IRQ > >> + struct irq_desc *DESC > >> , ...) { > >> + unsigned int IRQ = DESC->irq; > >> ... > >> - struct irq_desc *DESC = irq_to_desc(IRQ); > >> ... > >> } > >> > >> @ @ > >> identifier DECL.METHOD; > >> identifier IRQ; > >> identifier DESC; > >> @@ > >> METHOD( > >> - unsigned int IRQ > >> + struct irq_desc *DESC > >> , ...) { > >> + unsigned int IRQ = DESC->irq; > >> ... > >> - struct irq_desc *DESC; > >> ... > >> - DESC = irq_to_desc(IRQ); > >> ... > >> } > >> > >> @ @ > >> identifier DECL.METHOD; > >> identifier IRQ; > >> @@ > >> METHOD( > >> - unsigned int IRQ > >> + struct irq_desc *desc > >> , ...) { > >> + unsigned int IRQ = desc->irq; > >> ... > >> } > >> > >> @ @ > >> identifier DECL.METHOD; > >> identifier FUNC; > >> identifier IRQ; > >> @@ > >> FUNC(...) { > >> <... > >> METHOD( > >> - IRQ > >> + irq_to_desc(IRQ) > >> , ... ) > >> ...> > >> } > > > > I don't think FUNC(...) { <... and ...> } are needed here. The goal is to > > make the change everywhere the call appears. > > That is a reasonable simplification. > > >> @ @ > >> identifier FUNC; > >> identifier DESC; > >> identifier IRQ; > >> @@ > >> FUNC(..., struct irq_desc *DESC, ...) { > >> ... > >> unsigned int IRQ = DESC->irq; > >> <... > >> - irq_to_desc(IRQ) > >> + DESC > >> ...> > >> } > > > > This rule can be extended as follows: > > Nice. I had not picked up on the or operator. > > > @ @ > > identifier FUNC; > > identifier DESC; > > identifier IRQ; > > identifier FLD; > > @@ > > FUNC(..., struct irq_desc *DESC, ...) { > > ... > > unsigned int IRQ = DESC->irq; > > <... > > ( > > - irq_to_desc(IRQ) > > + DESC > > | > > - irq_desc[IRQ].FLD > > + DESC->FLD > > ) > > ...> > > } > > > > Doing so gets rid of more references to IRQ. > > Very reasonable, especially on arches like alpha where none of the > sparse irq work has hit. > > > > Another case that can be treated is method calls via a pointer: > > I didn't actually find any cases where that rule hit on arch/x86 > so I did not include it, but it makes sense. > > > @@ > > expression arg; > > struct irq_desc *desc; > > struct irq_chip *ic; > > identifier fun; > > @@ > > > > ( > > desc->chip->fun( > > - arg > > + desc > > ,...) > > | > > ic->fun( > > - arg > > + irq_to_desc(arg) > > ,...) > > ) > > > My paranoid sense says this rule should be: > @ @ > expresion IRQ_EXPR; > struct irq_desc *DESC; > struct irq_chip *CHIP; > identifier METHOD; > @@ > ( > DESC->chip->METHOD( > - IRQ_EXPR > + irq_to_desc(IRQ_EXPR) > , ... ) > | > CHIP->METHOD( > - IRQ_EXPR > + irq_to_desc(IRQ_EXPR) > , ... )
Agreed. But then it would be nice to check whether DESC is already computed as irq_to_desc(IRQ_EXPR).
julia
> If this is before my irq_to_desc removal rule. We should not > see any new irq_to_desc calls popping up. > > > julia > > > >> @ @ > >> identifier FUNC; > >> identifier DESC; > >> identifier IRQ; > >> @@ > >> FUNC(..., struct irq_desc *DESC, ...) { > >> ... > >> - unsigned int IRQ = DESC->irq; > >> ... when != IRQ > >> } >
| |