On 11/15/2011 11:31 PM, Maxim Kuvyrkov wrote: > I have eye-balled this patch for good half-an-hour and couldn't poke any > holes in it. I can't approve this patch, but below are some review comments. > Mostly these are suggested comments to make reload easier to understand for > future generations.
Thanks! >> ! /* Restore the constraint pointers to the previous >> ! alternative. */ >> ! for (i = 0; i < noperands; i++) >> ! constraints[i] = old_constraints[i]; > > I'm not sure the comment is precise. We are still on the current > alternative, we are just rolling back the advancement constraints[] done ... We are on the current alternative but the constraints array already points to the next and has to be rewinded. But I agree that the comment is confusing. However, I'll remove this anyway since as you said old_constraints is probably not needed at all. >> + /* Make a backup of the old constraint pointer since we >> + will need it when retrying the alternative with >> + swapped operands. */ >> + old_constraints[i] = constraints[i]; >> constraints[i] = p; > > ... here. > > Furthermore, as constraints[i] seem not be used down the function, can't we > just condition constraints[i] update on "if (swapped == (commutative >= 0 ? 1 > : 0))" and avoid old_constraints altogether? Right. I'll remove this. > Future generations would really appreciate comments about what is going on ... Ok. I'll add a comment saying that this just (un)swaps fields in different operand related arrays. Bye, -Andreas-
