Re: [PATCH] commit: Remove backward goto in read_craft_line()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Ralf Thielow <ralf.thielow@xxxxxxxxxxxxxx> writes:

>> If "--show-c-function" output is the problem, perhaps we should know a bit
>> better about what C function header looks like?
>
> In fact the "--show-c-function" output is the problem. But I think that
> a change can't be rejected because of another issue.

Well, I never said anything about rejection nor acceptance.

> The style of placing "goto"-statements, which leave a function to the
> end of that is used in many other projects. And I think
> it's very usefull.

It is a good discipline to follow in general to place an exceptional case
at the end if you jump out of the general flow.  But because the affected
function was so small, its readability doesn't benefit very much from the
discipline (in other words, you knew about a good discipline, but applied
it to a wrong function).  The patch was small and obviously correct, so it
will not hurt, but it will not make the world greatly a better place,
either.

IOW, it was a "Meh" topic for me.

It was more important to discuss Jonathan's "leave SP at the beginning of
a goto label to please --show-c-function" from the maintainer's point of
view, as people may remember it as a rule and start sending patches that
follow it, which I will need to deal with in the future.  I do not think
that one is a good rule.

Now that we have dealt with that more important business of letting people
know that protecting goto labels with a leading SP is _not_ the rule ;-),
I am happy with this discussion.

And often I forget about the original issue when a discussion reaches this
stage of happiness.  So thanks for reminding me.

As I said, even though I do not think the particular function you touched
badly needs the discipline applied, it would not hurt, and it obviously is
the right thing to do (iow, if the function were written from day one in a
way your patch reorganized it, we would never accept a patch to change it
into today's shape of jumping backwards).  For one thing, it would remove
an example from the codebase people can point at to make excuses when
responding to a review of their patch that adds a backward goto to a much
larger function.

Will queue after massaging the log message somewhat.  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


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]