Messages in this thread | | | Subject | Re: [PATCH] coccinelle: misc: add swap script | From | Denis Efremov <> | Date | Thu, 18 Feb 2021 14:34:04 +0300 |
| |
On 2/18/21 2:29 PM, Julia Lawall wrote: > > > On Thu, 18 Feb 2021, Denis Efremov wrote: > >> >> >> On 2/18/21 1:17 PM, Julia Lawall wrote: >>> >>> >>> On Thu, 18 Feb 2021, Denis Efremov wrote: >>> >>>> >>>> >>>> On 2/18/21 12:31 AM, Julia Lawall wrote: >>>>>> +@depends on patch@ >>>>>> +identifier tmp; >>>>>> +expression a, b; >>>>>> +type T; >>>>>> +@@ >>>>>> + >>>>>> +( >>>>>> +- T tmp; >>>>>> +| >>>>>> +- T tmp = 0; >>>>>> +| >>>>>> +- T *tmp = NULL; >>>>>> +) >>>>>> +... when != tmp >>>>>> +- tmp = a; >>>>>> +- a = b; >>>>>> +- b = tmp; >>>>>> ++ swap(a, b); >>>>>> +... when != tmp >>>>> >>>>> In this rule and the next one, if you remove the final ; from the b = tmp >>>>> line and from the swap line, and put it into context code afterwards, them >>>>> the generated code looks better on cases like fs/xfs/xfs_inode.c in the >>>>> function xfs_lock_two_inodes where two successive swap calls are >>>>> generated. >>>>> >>>>> There are also some cases such as drivers/net/wireless/ath/ath5k/phy.c in >>>>> the function ath5k_hw_get_median_noise_floor where the swap code makes up >>>>> a whole if branch. >>>> >>>>> In this cases it would be good to remove the {}. >>>> >>>> How this can be handled? >>>> >>>> If I use this pattern: >>>> @depends on patch@ >>>> identifier tmp; >>>> expression a, b; >>>> @@ >>>> >>>> ( >>>> if (...) >>>> - { >>>> - tmp = a; >>>> - a = b; >>>> - b = tmp >>>> + swap(a, b) >>>> ; >>>> - } >>>> | >>>> - tmp = a; >>>> - a = b; >>>> - b = tmp >>>> + swap(a, b) >>>> ; >>>> ) >>>> >>>> The tool fails with error: >>>> >>>> EXN: Failure("rule starting on line 58: already tagged token:\nC code >>>> context\nFile \"drivers/net/wireless/ath/ath5k/phy.c\", line 1574, >>>> column 4, charpos = 41650\n around = 'sort',\n whole content = >>>> \t\t\t\tsort[j - 1] = tmp;") in drivers/net/wireless/ath/ath5k/phy.c >>> >>> A disjunction basically says "at this node in the cfg, can I match the >>> first patter, or can I match the second pattern, etc." Unfortunately in >>> this case the two branches start matching at different nodes, so the short >>> circuit aspect of a disjunction isn't used, and it matches both patterns. >>> >>> The solution is to just make two rules. The first for the if case and the >>> second for everything else. >>> >> >> if (...) >> - { >> - tmp = a; >> - a = b; >> - b = tmp >> + swap(a, b) >> ; >> - } >> >> >> This produces "single-line ifs". >> Maybe generated patches can be improved somehow? >> Moving -+; doesn't help in this case. > > There is clearly some problem with the management of newlines... > > The other alternative is to just have one rule for introducing swap and > another for removing the braces around a swap, ie > > if (...) > - { > swap(...); > - } > > I don't think it would be motivated to remove the newline in that case.
Yes, this works. I'll send v2.
Thanks
| |