Re: [Xen-devel] differing opinions between maintainers vs patch acks

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/05/17 13:54, Jan Beulich wrote:
>>>> On 04.05.17 at 14:44, <ian.jackson@xxxxxxxxxxxxx> wrote:
>> Andrew Cooper writes ("Re: differing opinions between maintainers vs patch 
>> acks"):
>>> Taking this example, as you have called it out, but without going into
>>> the details.
>>>
>>> I accept that the issues under debate do not have any impact on the
>>> technical correctness of the fix.  Once compiled/assembled, the binary
>>> will function correctly.
>>>
>>> However, the bikeshedding makes a very real material impact on the
>>> understandability and reviewability of the code.
>>>
>>> In my mind, all other things being equal, making the code easier to
>>> understand and review is of paramount importance, and particularly in
>>> this case, not making an already complicated bit of code harder to review.
>> Well, at one level I agree with Andrew on at least the 1*1 and 0*8
>> question.  These seem clearer to me as they state the programmer's
>> intent as well as merely the effect.  I found Jan's response hard to
>> understand; there doesn't actually seem to be a counterargument.
> My counterargument was that 0*8 clearly equals 0 for anyone
> knowledgeable enough to read this code, as does 1*8 = 8.
> Anyway, seeing that you agree with Andrew, I'll go make the
> change, no matter that I think it doesn't belong here (besides
> being pointless).

Right, so the underlying issue here is the subjective nature of whether
this change is pointless or not.

You have made and argument for the changes being pointless, and I have
made an argument for the changes not being pointless.

For this type of problem, would it help if everyone made a more
conscious effort to work out when a subjective deadlock has been
reached, and try to actively involve a 3rd party to tie break?

>
>>  I
>> suspect if I thought about it enough I would agree with Andrew about
>> the labels too.
> Along those lines I'd then also go make the change here, if only
> there was an alternative naming of the label tags that I can at
> least live with; the suggestion to simply divide the numbers by 8
> is, as expressed, not something I consider reasonable. So I'll
> make my changing of those label tags dependent one someone
> coming forward with a naming scheme which is both better than
> what is there and better then using simple stack slot numbers
> without it being clear that stack slots are being meant.

I am afraid I cannot offer a helpful naming alternative beyond what has
already been discussed.

However, a comment explaining the meaning of the number suffixes would
go a very long way towards aiding the understandability of the code, at
which point leaving them as they are would be ok.

There are a lot of cases where I am happy with leaving the code "no
worse than it was before" (and I do try to identify these cases as they
appear during review), but the root of my objection in this case is my
(subjective) view that changes take an already-hard-to-understand piece
of code and make it worse by introducing inconsistencies in the way that
important pieces of information are expressed.

I am sorry this has flown so much out of proportion, especially as my
XSA followup which reimplements this in C was almost ready to be posted
to xen-devel already.  I am not deliberately trying to be awkward, but I
do care about improving the quality of the codebase, and it is clear
that we have different opinions on what qualifies as "obvious".

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel