Messages in this thread | | | Subject | Re: hwmon/sch5627: Use common error handling code in sch5627_probe() | From | Hans de Goede <> | Date | Tue, 13 Mar 2018 09:52:57 +0100 |
| |
Hi,
On 13-03-18 09:25, SF Markus Elfring wrote: >>> Adjust jump targets so that a bit of exception handling can be better >>> reused at the end of this function. > … >> goto-s going to a label calling another goto is completely unreadable. > > I got an other software development view. > > >> I really do not see any reason for the proposed changes, > > I suggest to look once more. > > >> they may remove a small amount of code duplication, > > This was my software design goal in this case.
The diffstat of your patch is:
1 file changed, 29 insertions(+), 31 deletions(-)
So you are asking people to review 60 changed lines to save 2, that alone should be the point where you stop yourself from *even* sending this patch. Review time is not an endless free resource and this patch is wasting it for very little gain.
I see in the kernel git log that you've e.g. also send a lot of patches removing pr_err / dev_err calls from memory allocation failures. Those are good patches to have, they are easy to review and significantly shrink the compiled kernel size because of all the text strings you are removing.
This patch however will likely result in a binary which is hardly smaller at all (if at all, the compiler does common code elimination itself) while it is a complex patch to review so comes with a large review cost.
Next time before you send a patch please carefully think if the saving is worth the combination of reviewers time + the risk of regressions (and keep in mind that both the reviewers time and the risk of regressions cost increase for more complex changes).
>> but at a hugh cost wrt readability. > > I proposed a different trade-off here.
<sigh> not this again. Cleanup patches are appreciated, but there always is a cost to making changes to perfectly working good code.
You've had this same discussion with Jonathan Cameron, the IIO subsys maintainer at the end of 2017, it would be nice if you were to actually listen to what people are saying about this.
As for this specific discussion, there are certain "design-patterns" in the kernel, goto style error handling is one of them, the pattern there ALWAYS is:
error_cleanup4: cleanup4(); /* fall through to next cleanup */ error_cleanup3: cleanup3(); /* fall through to next cleanup */ error_cleanup2: cleanup2(); /* fall through to next cleanup */ error_cleanup1: cleanup1(); /* fall through to next cleanup */ return error;
Notice the fall-thoughs those are ALWAYS there, never, ever is there a goto after a cleanup label. The idea is that resources are allocated in the order of resource1 (cleaned by cleanup1()) then resource2, etc. and the goto + fallthrough will cleanup all resources which have been allocated at the time of the goto in *reverse* order of allocation. The whole point of this design- pattern to be able to a) do partial cleanup if we fail halfway through; b) do it in reverse order. Your patches black goto magic completely messes this up and clearly falls under the CS101 rule: never use goto.
Your proposed changes break how every experienced kernel developer is expecting the code to be / work, causing your proposed changes to have a HUGE cost wrt maintainability and readability, which means the trade-off you're suggesting is anything but worth-while.
TL;DR: still NACK.
Regards,
Hans
| |