At Tue, 6 Feb 2018 11:56:27 +0530, Amit Khandekar <amitdkhan.pg@gmail.com> wrote in
<CAJ3gD9fJhKBHn-fx6UzYZeLa0N8WnnZb2BsUTm_5A9kYsJgnww@mail.gmail.com>
> On 6 February 2018 at 10:11, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> >> At Mon, 5 Feb 2018 15:29:27 +0530, Amit Khandekar <amitdkhan.pg@gmail.com> wrote in
> >> > Attached is a patch that fixes this issue on the above lines.
> >>
> >> The patch adds two new variables and always sets them but warp
> >> around can happen at most once per call so I think it is enough
> >> to arrange to stop at the wrap around time. Addition to that the
> >> patch is forgetting the case of no partial plan. The loop can
> >> overrun on pa_finished in the case.
> >
> > Sorry, the patch works fine. But still the new variables don't
> > seem needed.
>
> True, I could have set initially as_whichplan to pa_next_plan, and
> used as_whichplan instead of initial_plan. And, I could have used the
> condition initial_plan >= append->first_partial_plan instead of
> should_wrap_around. The reason I used these two variables is because I
> thought the logic would be more reader-friendly. (Also,
> should_wrap_around uses static values so using this variable avoids
> determining the condition (initial_plan >= append->first_partial_plan)
> at each iteration).
Thanks for the explanation. I'm fine with it. Well may I make
some comments on the patch?
- It seems to me that the if (!should_warp_around) block and else
block are better be transposed so that make the bail out code
closer to the exit condition check as the previous code
was. (In other was the second block should be if
(should_wrap...), not if (!should_warp..).
- The following comment needs a "the" before the "first", maybe.
+ /* Loop back to first partial plan. */
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center