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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

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.

In my case I've suggested adding a comment to clue people into the
fact that the postfix semantics are in operation; I think that
balances "reducing cognitive load" with "avoids unnecessarily verbose
code".

Other options would be things like this:

do {
 i--;
 [cleanup]
} while ( i > 0 );

or

while ( i > 0 ) {
 i--;
 [cleanup]
}

The first one I think is the clearest, but neither one are very concise.

 -George

[1] marc.info/?i=<56EBC5E102000078000DE376@xxxxxxxxxxxxxxxxxxxxxxx>

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