Re: [Cocci] s390/net: Deletion of unnecessary checks before two function calls
> If you can benchmark the code and the new code is faster then, yes, this > patch is good and we will apply it. I guess that I do not have enough resources myself to measure different run time effects in a S390 environment. > If you have no benchmarks then do not send the patch. Are other software developers and testers eventually interested to try a few pointer check adjustments out a bit more? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] s390/net: Deletion of unnecessary checks before two function calls
On Mon, 3 Nov 2014, Derek M Jones wrote: > Dan > > > The truth is I think that all these patches are bad and they make the > > code harder to read. > > I disagree, I think the code requires less effort to read without the > if test. > > A developer reading the code will wonder why kfree does not handle the > case when its argument is NULL. This takes effort. > > Now there might be a reason while kfree (or any other function) does > not handle NULL, in which case the test is necessary for that reason. > Or perhaps calling kfree has other consequences and this means it is > good to minimise the number of calls, fair enough. This may be true in general, but the standard assumption about kernel functions is that they are not defensive. Everything used in the kernel is defined in the kernel, and is normally written in a way to be optimal for in-kernel usage. > > The if statements are there for *human* readers to understand and you are > > making it harder for humans to understand the code. > > The reverse is true. > > But if there are other reasons, then leave the test in. The point about a null test is that it makes it apparent at the current point that the value can be null. The default assumption is that it cannot, ie that functions are only called when doing so is useful. Actually, this assumption can be exploited by automatic tools for finding out what needs to be done when: Suman Saha, Jean-Pierre Lozi, Gaël Thomas, Julia L. Lawall, Gilles Muller: Hector: Detecting Resource-Release Omission Faults in error-handling code for systems software. DSN 2013: 1-12 The point is that Linux code contains a huge number of alloc and free functions, many of which are not very well known. If free functions are only called when they are useful, then you can infer which free functions go with which alloc functions. If all free functions are just lumped together and called at random, then you lose a lot of information. Similarly, as a human, when I look at code I don't know, it is very confusing to have functions called when as far as I can tell they don't need to be. A bunch of validity tests at the beginning of the called function doesn't really help, because as Dan points out the code is not always easy to find, and in the function definition itself one doesn't know in what contexts that code is intended to be used. In separate cleanup functions (destroy_xxx) perhaps the tests are more or less noise. But in the error handling code of an initialization function, no test means to me that the call is needed at the current point, and a test means to me that for some reason it is not statically known whether a call is needed or not. This is information that is really useful for understanding the code, for both humans and for tools. julia___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] s390/net: Deletion of unnecessary checks before two function calls
Dan The truth is I think that all these patches are bad and they make the code harder to read. I disagree, I think the code requires less effort to read without the if test. A developer reading the code will wonder why kfree does not handle the case when its argument is NULL. This takes effort. Now there might be a reason while kfree (or any other function) does not handle NULL, in which case the test is necessary for that reason. Or perhaps calling kfree has other consequences and this means it is good to minimise the number of calls, fair enough. The if statements are there for *human* readers to understand and you are making it harder for humans to understand the code. The reverse is true. But if there are other reasons, then leave the test in. -- Derek M. Jones tel: +44 (0) 1252 520 667 Knowledge Software Ltd blog:shape-of-code.coding-guidelines.com Software analysis http://www.knosof.co.uk ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] s390/net: Deletion of unnecessary checks before two function calls
On Mon, Nov 03, 2014 at 05:50:48PM +0100, SF Markus Elfring wrote: > > After your patch then it will print warning messages. > > To which messages do you refer to? > Open your eyeballs up and read the code. > > > The truth is I think that all these patches are bad and they make the > > code harder to read. > > > > Before: The code is clear and there is no NULL dereference. > > Where do you stumble on a null pointer access? > I'm not talking about bugs, I'm talking about code clarity. Before is more clear than after. > > > After: You have to remember that rtw_free_netdev() accepts NULL > > pointers but free_netdev() does not accept NULL pointers. > > Are any improvements needed for the corresponding documentation to make it > better accessible besides the source code? > Documentation doesn't reduce the number of things to remember it just documents it. Meanwhile if we leave the code as-is there is no need for documentation because the code is clear. > > > The if statements are there for *human* readers to understand and you are > > making it harder for humans to understand the code. > > Is there a target conflict between source code understandability > and software efficiency? If you can benchmark the code and the new code is faster then, yes, this patch is good and we will apply it. If you have no benchmarks then do not send the patch. > > > Even for kfree(), just removing the if statement is not really the right > > fix. We do it because everyone knows kfree(), but what Julia Lawall > > said is the real correct way change the code and make it simpler for > > people to understand: > > > > https://lkml.org/lkml/2014/10/31/452 > > You refer to another update suggestion for the software area > "staging: rtl8188eu". > Do you find adjustments for jump labels easier to accept than the simple > deletion of specific null pointer checks? Yes. > > > > I know it's fun to send automated patches but these make the code worse > > and they waste reviewer time. > > I hope that small automated changes can also help to improve affected > source files. No. The changes make the code less clear. regards, dan carpenter ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] s390/net: Deletion of unnecessary checks before two function calls
> > After your patch then it will print warning messages. > > After: You have to remember that rtw_free_netdev() accepts NULL > > pointers but free_netdev() does not accept NULL pointers. > > Are any improvements needed for the corresponding documentation to make it > better accessible besides the source code? When people are writing or reading code, they will not necessarily look at the documentation for every function that they use. > > The if statements are there for *human* readers to understand and you are > > making it harder for humans to understand the code. > > Is there a target conflict between source code understandability > and software efficiency? Efficiency is not an issue. This code is all in rare error handling paths or in service removal functions. None of it is in a critical path. What is important is to be able to easily check that what needs to be done is actually done. Removing null tests makes it more obscure what needs to be done, because it means that the conditions under which a function needs to be called (which may be different than the conditions under which it can be called) are less apparent. julia ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] s390/net: Deletion of unnecessary checks before two function calls
> After your patch then it will print warning messages. To which messages do you refer to? > The truth is I think that all these patches are bad and they make the > code harder to read. > > Before: The code is clear and there is no NULL dereference. Where do you stumble on a null pointer access? > After: You have to remember that rtw_free_netdev() accepts NULL >pointers but free_netdev() does not accept NULL pointers. Are any improvements needed for the corresponding documentation to make it better accessible besides the source code? > The if statements are there for *human* readers to understand and you are > making it harder for humans to understand the code. Is there a target conflict between source code understandability and software efficiency? > Even for kfree(), just removing the if statement is not really the right > fix. We do it because everyone knows kfree(), but what Julia Lawall > said is the real correct way change the code and make it simpler for > people to understand: > > https://lkml.org/lkml/2014/10/31/452 You refer to another update suggestion for the software area "staging: rtl8188eu". Do you find adjustments for jump labels easier to accept than the simple deletion of specific null pointer checks? > I know it's fun to send automated patches but these make the code worse > and they waste reviewer time. I hope that small automated changes can also help to improve affected source files. Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] s390/net: Deletion of unnecessary checks before two function calls
On Mon, Nov 03, 2014 at 05:10:35PM +0100, SF Markus Elfring wrote: > > I agree with your proposed debug_unregister() changes, but not with your > > kfree_fsm() change. > > Why do you want to keep an additional null pointer check before the call > of the kfree_fsm() function within the implementation of the > netiucv_free_netdevice() function? Think about how long it takes you to figure this out what the bug is and then remember that we have to spend that same amount of time multiplied by the number of patches you have sent. regards, dan carpenter ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] s390/net: Deletion of unnecessary checks before two function calls
On Mon, Nov 03, 2014 at 04:55:12PM +0100, SF Markus Elfring wrote: > > This one is buggy. > > I am still interested to clarify this opinion a bit more. > After your patch then it will print warning messages. The truth is I think that all these patches are bad and they make the code harder to read. Before: The code is clear and there is no NULL dereference. After: You have to remember that rtw_free_netdev() accepts NULL pointers but free_netdev() does not accept NULL pointers. The if statements are there for *human* readers to understand and you are making it harder for humans to understand the code. Even for kfree(), just removing the if statement is not really the right fix. We do it because everyone knows kfree(), but what Julia Lawall said is the real correct way change the code and make it simpler for people to understand: https://lkml.org/lkml/2014/10/31/452 I know it's fun to send automated patches but these make the code worse and they waste reviewer time. regards, dan carpenter ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] s390/net: Deletion of unnecessary checks before two function calls
> This one is buggy. I am still interested to clarify this opinion a bit more. > I'm sorry, but please stop sending these. I am going to improve more implementation details in affected source files. > But for this one: > 1) I don't know what the functions do so I have to look at the code. I hope that static source code analysis can help here. > 2) It's in a arch that I don't compile so cscope isn't set up meaning >it's hard to find the functions. Do you find the Coccinelle software also useful for your area? > You're sending a lot of patches and they are all hard to review and some > of them are buggy and none of them really add any value. Thanks for your feedback. The suggested source code clean-up might result in a measurable effect depending on the call frequency for the changed functions. Can I help you in any ways to make corresponding review easier? > It's a waste of your time and it's a waste of my time. It can be your choice to reject my update suggestion. Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] s390/net: Deletion of unnecessary checks before two function calls
> I agree with your proposed debug_unregister() changes, but not with your > kfree_fsm() change. Why do you want to keep an additional null pointer check before the call of the kfree_fsm() function within the implementation of the netiucv_free_netdevice() function? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
