Hi,
On 25-10-17 18:58, SF Markus Elfring wrote:
If that is the only unlock in the function, then it is probably
best to keep things as is. In general gotos are considered
better then multiple unlocks, but not having either is
even better.
Thanks for your quick feedback.
How do you think about to use the following code variant then?
if (!ret)
ret = IIO_VAL_INT;
I believe the goto unlock variant and setting ret = IIO_VAL_INT;
directly above the unlock label variant is better,
I would prefer the approach above so that an extra goto statement
could also be avoided before.
Usually code in a function follows a pattern of:
err = step1()
if (err)
handle-err
err = step2()
if (err)
handle-err
err = step3()
if (err)
handle-err
What you are suggesting breaks this pattern (not
using a goto in the last if (err) case) which makes
the code harder to read and makes things harder
(and potentially leads to introducing bugs) when
a step4() gets added.
because that way the error handling is consistent between all steps
and if another step is later added at the end, the last step will
not require modification.
Do any more contributors insist on such a code layout?
There definitely is some personal preference involved here,
but I do believe that consistency is more important then
saving a goto here.
How long should I wait for corresponding feedback before another small
source code adjustment will be appropriate?
That is hard to say.
I am just curious on how we can achieve progress here.
I usually just do a new version when I've time,
This is generally fine.
seldomly someone complains I should have waited longer for feedback
(when I'm quite quick) but usually sending out a new version as soon
as you've time to work on a new version is best, since if you wait
you may then not have time for the entire next week or so, at least
that is my experience :) There is really no clear rule here.
I asked also because other well-known contributors showed strong
reactions for this change pattern (with the help of a script
for the semantic patch language).
Would you care for similar updates in source files like the following?
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/accel/kxcjk-1013.c?id=1540d0106bcbc4e52013d759a0a0752ae7b4a09d#n760
So I just checked this one, this one is tricky because the
lock is taking inside a switch-case and doing gotos inside
the case is not pretty. If I were to refactor this, I would
add an "if (mask == IIO_CHAN_INFO_SCALE) {}" block to
handle the unlocked scale case and then take the lock around
the entire switch-case, using breaks on error to jump to
the unlock after the switch-case without needing gotos.
To me this seems the right thing here, since the scale is
special here in that it does not need locking. Or optionally
one can just take the lock for scale regardless, it won't
hurt (much).
Basically I believe there is no one-size fits all solution
here and refactoring like this may introduce bugs, so one
needs to weight the amount of work + regression risk vs
the benefits of the code being cleaner going forward.
Regards,
Hans
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/accel/stk8312.c?id=36ef71cae353f88fd6e095e2aaa3e5953af1685d#n432
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/adc/palmas_gpadc.c?id=36ef71cae353f88fd6e095e2aaa3e5953af1685d#n304
Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
