Re: [Xen-devel] [PATCH v4 08/34] vmap: Make the while loop less fishy.

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Mon, Mar 21, 2016 at 1:26 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 21.03.16 at 13:04, <George.Dunlap@xxxxxxxxxxxxx> wrote:
>> On Thu, Mar 17, 2016 at 4:08 PM, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> 
>> wrote:
>>> Konrad Rzeszutek Wilk writes ("[PATCH v4 08/34] vmap: Make the while loop 
>>> less fishy."):
>>>>   error:
>>>> -    while ( i-- )
>>>> -        free_domheap_page(mfn_to_page(mfn_x(mfn[i])));
>>>> +    while ( i )
>>>> +        free_domheap_page(mfn_to_page(mfn_x(mfn[--i])));
>>>
>>> I quite strongly dislike this.  It is good practice to keep the loop
>>> control code together where this is reasonably convenient.
>>>
>>> I wouldn't quibble on such a stylistic matter (particularly outside my
>>> bailiwick) but (a) I would like to reinforce Jan's position and
>>> (b) it seems worth writing an email as there will be many occurrences.
>>
>> Since we're taking about general principle (and I've been referred to
>> here from a similar discussion elsewhere [1]), let me weigh in as
>> well.
>>
>> I can see the point of not wanting the decrement to be in the middle
>> of the expression here.  But I also entirely agree with Konrad's
>> assessment that this code is likely to be confusing; and the fact that
>> a computer program following a list of rules *developed by
>> professional bug-finders* is confused by this kind of semantics I
>> think supports this assessment.  At very least it has the potential to
>> waste a lot of mental energy figuring out why code that looks wrong
>> isn't wrong; and at worst there's a risk that at some point someone
>> will "fix" it incorrectly.
>>
>> The fact that there are already many instances of this pattern in the
>> source tree would be relevant if we expect nobody but people currently
>> familiar with the code to every try to read or modify it.  But since
>> on the contrary we hope that others will contribute to the codebase,
>> and even that they may eventually become maintainers, I think there is
>> sense in addressing them, at least as they come up.
>
> Well, if talk was about something really complex here, I might
> agree. But unary prefix and postfix operators are an integral
> part of the C language, and while code reading indeed shouldn't
> require overly much mental energy, I think we should be
> permitted to make full knowledge of the base programming
> language a prereq to reading our code. Otherwise - where do
> you want to draw the boundary of what is permitted and what
> is not? (Yes, I know I'm guilty in occasionally writing rather
> complex expressions, with at times not immediately obvious side
> effects, and I'm trying to do better irrespective of all such also
> falling in the above "basic language" features category. That's
> because I can see doing so being past the boundary of
> reasonably understandable code.)

And I'm not saying that we can't take advantage of postfix operators
in this case; I'm just saying that if we are going to, it would be
better to point it out in a comment.

There are lots of "features" of C that standard practice still demands
that we add a comment for; the default fall-through for switch
statements comes to mind.  Of course programmers should be required to
know that switch statements default to fallthrough in C; but it's
still a common mistake to forget that, and so we point them in the
right direction by adding comments.

Similarly, of course programmers should know the difference between
prefix and postfix operators.  But this is a case where there's a risk
of tripping over something, so it makes sense to point them in the
right direction by adding comments.

Or to take a different tack: I understand that you don't think there's
no particular benefit to adding a comment in cases like this; could
you explain to me why you think it would have a significant cost?

>> do {
>>  i--;
>>  [cleanup]
>> } while ( i > 0 );
>>
>
> But you realize that the first (but not the second) one is wrong for
> the i == 0 case?

So it is.  I think I was thinking of a case where it was known that at
least one iteration had succeeded; but obviously the second is more
safe in general.

 -George

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