* Re: [patch 119/197] scripts/coccinelle/free: Delete NULL test before freeing functions
@ 2014-08-09 8:37 ` Julia Lawall
0 siblings, 0 replies; 12+ messages in thread
From: Julia Lawall @ 2014-08-09 8:37 UTC (permalink / raw)
To: SF Markus Elfring
Cc: Andrew Morton, Coccinelle, linux-kernel, kernel-janitors,
Fabian Frederick, Joe Perches
On Sat, 9 Aug 2014, SF Markus Elfring wrote:
> > +@r depends on context || report || org @
> > +expression E;
> > +position p;
> > +@@
> > +
> > +* if (E)
> > +* \(kfree@p\|debugfs_remove@p\|debugfs_remove_recursive@p\|usb_free_urb\)(E);
>
> How do you think about to add the construct "@p" also behind the function name
> "usb_free_urb"?
Yes, that would be needed, thanks. Also, it was recently suggested to add
of_node_put to the list of things that are checked for.
> Is my previous update suggestion for such an issue worth for another look, too?
I still don't think this should be done for any random function that
performs a null test on its argument. The corrections involved here are
not as trivial as they would seem. Often it is not the case that the null
test at the call site should be just deleted, instead the code should be
reorganized. Making a rule that treats 5000 functions that are
automatically selected will just encourage people to do sloppy things.
(Personally, I don't like the whole null test removal idea. It mixes the
notion of something that is necessary and has failed and something that is
optional. In the case of something that is necessary and has failed, the
null value is usually statically apparent, and the code can be reorganized
so that a null value doesn't go where it is not wanted. In the case of
something that is simply optional, the presence of the null test gives
the reader of the code some information. Removing this information in
rarely executed code seems unfortunate.)
julia
>
> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch 119/197] scripts/coccinelle/free: Delete NULL test before freeing functions
@ 2014-08-09 8:37 ` Julia Lawall
0 siblings, 0 replies; 12+ messages in thread
From: Julia Lawall @ 2014-08-09 8:37 UTC (permalink / raw)
To: cocci
On Sat, 9 Aug 2014, SF Markus Elfring wrote:
> > +@r depends on context || report || org @
> > +expression E;
> > +position p;
> > +@@
> > +
> > +* if (E)
> > +* \(kfree@p\|debugfs_remove@p\|debugfs_remove_recursive@p\|usb_free_urb\)(E);
>
> How do you think about to add the construct "@p" also behind the function name
> "usb_free_urb"?
Yes, that would be needed, thanks. Also, it was recently suggested to add
of_node_put to the list of things that are checked for.
> Is my previous update suggestion for such an issue worth for another look, too?
I still don't think this should be done for any random function that
performs a null test on its argument. The corrections involved here are
not as trivial as they would seem. Often it is not the case that the null
test at the call site should be just deleted, instead the code should be
reorganized. Making a rule that treats 5000 functions that are
automatically selected will just encourage people to do sloppy things.
(Personally, I don't like the whole null test removal idea. It mixes the
notion of something that is necessary and has failed and something that is
optional. In the case of something that is necessary and has failed, the
null value is usually statically apparent, and the code can be reorganized
so that a null value doesn't go where it is not wanted. In the case of
something that is simply optional, the presence of the null test gives
the reader of the code some information. Removing this information in
rarely executed code seems unfortunate.)
julia
>
> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Cocci] [patch 119/197] scripts/coccinelle/free: Delete NULL test before freeing functions?
2014-08-09 8:37 ` Julia Lawall
(?)
@ 2014-08-09 9:13 ` SF Markus Elfring
-1 siblings, 0 replies; 12+ messages in thread
From: SF Markus Elfring @ 2014-08-09 9:13 UTC (permalink / raw)
To: cocci
> I still don't think this should be done for any random function that
> performs a null test on its argument. The corrections involved here are
> not as trivial as they would seem.
I would prefer to make the list of corresponding function names more complete.
> Often it is not the case that the null test at the call site should be
> just deleted, instead the code should be reorganized.
Which source code places do you know where a different approach might look better?
> (Personally, I don't like the whole null test removal idea. [...]
Would you like to clarify involved software concerns a bit more?
Regards,
Markus
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch 119/197] scripts/coccinelle/free: Delete NULL test before freeing functions?
@ 2014-08-09 9:13 ` SF Markus Elfring
0 siblings, 0 replies; 12+ messages in thread
From: SF Markus Elfring @ 2014-08-09 9:13 UTC (permalink / raw)
To: Julia Lawall
Cc: Andrew Morton, Coccinelle, linux-kernel, kernel-janitors,
Fabian Frederick, Joe Perches
> I still don't think this should be done for any random function that
> performs a null test on its argument. The corrections involved here are
> not as trivial as they would seem.
I would prefer to make the list of corresponding function names more complete.
> Often it is not the case that the null test at the call site should be
> just deleted, instead the code should be reorganized.
Which source code places do you know where a different approach might look better?
> (Personally, I don't like the whole null test removal idea. [...]
Would you like to clarify involved software concerns a bit more?
Regards,
Markus
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch 119/197] scripts/coccinelle/free: Delete NULL test before freeing functions?
@ 2014-08-09 9:13 ` SF Markus Elfring
0 siblings, 0 replies; 12+ messages in thread
From: SF Markus Elfring @ 2014-08-09 9:13 UTC (permalink / raw)
To: cocci
> I still don't think this should be done for any random function that
> performs a null test on its argument. The corrections involved here are
> not as trivial as they would seem.
I would prefer to make the list of corresponding function names more complete.
> Often it is not the case that the null test at the call site should be
> just deleted, instead the code should be reorganized.
Which source code places do you know where a different approach might look better?
> (Personally, I don't like the whole null test removal idea. [...]
Would you like to clarify involved software concerns a bit more?
Regards,
Markus
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Cocci] [patch 119/197] scripts/coccinelle/free: Delete NULL test before freeing functions
2014-08-09 8:37 ` Julia Lawall
(?)
@ 2015-05-06 13:45 ` SF Markus Elfring
-1 siblings, 0 replies; 12+ messages in thread
From: SF Markus Elfring @ 2015-05-06 13:45 UTC (permalink / raw)
To: cocci
>>> + at r depends on context || report || org @
>>> +expression E;
>>> +position p;
>>> +@@
>>> +
>>> +* if (E)
>>> +* \(kfree at p\|debugfs_remove at p\|debugfs_remove_recursive at p\|usb_free_urb\)(E);
>>
>> How do you think about to add the construct "@p" also behind the function name
>> "usb_free_urb"?
>
> Yes, that would be needed, thanks. Also, it was recently suggested to add
> of_node_put to the list of things that are checked for.
Would you like to achieve further improvements for this approach?
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/scripts/coccinelle/free/ifnullfree.cocci?id=refs/tags/v4.0.1#n34
Regards,
Markus
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch 119/197] scripts/coccinelle/free: Delete NULL test before freeing functions
@ 2015-05-06 13:45 ` SF Markus Elfring
0 siblings, 0 replies; 12+ messages in thread
From: SF Markus Elfring @ 2015-05-06 13:45 UTC (permalink / raw)
To: Julia Lawall
Cc: Andrew Morton, Coccinelle, linux-kernel, kernel-janitors,
Fabian Frederick, Joe Perches
>>> +@r depends on context || report || org @
>>> +expression E;
>>> +position p;
>>> +@@
>>> +
>>> +* if (E)
>>> +* \(kfree@p\|debugfs_remove@p\|debugfs_remove_recursive@p\|usb_free_urb\)(E);
>>
>> How do you think about to add the construct "@p" also behind the function name
>> "usb_free_urb"?
>
> Yes, that would be needed, thanks. Also, it was recently suggested to add
> of_node_put to the list of things that are checked for.
Would you like to achieve further improvements for this approach?
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/scripts/coccinelle/free/ifnullfree.cocci?id=refs/tags/v4.0.1#n34
Regards,
Markus
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch 119/197] scripts/coccinelle/free: Delete NULL test before freeing functions
@ 2015-05-06 13:45 ` SF Markus Elfring
0 siblings, 0 replies; 12+ messages in thread
From: SF Markus Elfring @ 2015-05-06 13:45 UTC (permalink / raw)
To: cocci
>>> +@r depends on context || report || org @
>>> +expression E;
>>> +position p;
>>> +@@
>>> +
>>> +* if (E)
>>> +* \(kfree@p\|debugfs_remove@p\|debugfs_remove_recursive@p\|usb_free_urb\)(E);
>>
>> How do you think about to add the construct "@p" also behind the function name
>> "usb_free_urb"?
>
> Yes, that would be needed, thanks. Also, it was recently suggested to add
> of_node_put to the list of things that are checked for.
Would you like to achieve further improvements for this approach?
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/scripts/coccinelle/free/ifnullfree.cocci?id=refs/tags/v4.0.1#n34
Regards,
Markus
^ permalink raw reply [flat|nested] 12+ messages in thread