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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 21.03.16 at 13:04, <George.Dunlap@eu.citrix.com> wrote:
> On Thu, Mar 17, 2016 at 4:08 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> 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.)

> 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.

But you realize that the first (but not the second) one is wrong for
the i == 0 case?

Jan


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