Jonathan Nieder <jrnieder@xxxxxxxxx> writes:
> Not having read the patch yet, the above makes me suspect this is
> going to make the code worse. A 'goto' for exception handling can
> be a clean way to ensure everything allocated gets released, and
> restructuring to avoid that can end up making the code more error
> prone and harder to read.
>
> In other words, the "goto" removal should be a side effect and not
> the motivation.
Yes, I shared the same general feeling (cf. $gmane/279405).
> More generally, the patch seems to be about changing from a code structure
> of
>
> if (condition) {
> handle it;
> goto done;
> }
> if (other condition) {
> handle it;
> goto done;
> }
> handle misc;
> goto done;
>
> to
>
> if (condition) {
> handle it;
> } else if (other condition) {
> handle it;
> } else {
> handle misc;
> }
>
> In this example the postimage is concise and simple enough that it's
> probably worth it, but it is not obvious in the general case that this
> is always a good thing to do.
Generally, a large piece of code is _easier_ to read with forward
"goto"s that jump to the shared clean-up code, as they serve as
visual cues that tell the reader "you can stop reading here and
ignore the remainder of this if/else if/... cascade".
> Now that I see the patch is already merged, I don't think it needs
> tweaks. Just a little concerned about the possibility of people
> judging from the commit message and emulating the pattern in the rest
> of git.
Yes, we shouldn't let people blindly imitate this change. I merged
it primarily because I wanted the change get out of my hair, as
other changes in flight started conflicting with it.
This kind of change can be good one only in a narrowly defined case
(like this one) but I agree that in general, as you said at the
beginning, it is an easy way to make the resulting code less
maintainable and harder to read.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html