On Wednesday 10 March 2010, Antonio Borneo wrote:
> On Wed, Mar 10, 2010 at 5:26 PM, David Brownell <davi...@pacbell.net> wrote:
> > Could you reduce the gratuitous changes which just hide
> > the content of your patch? Like shuffling comments, Such
> > things just make patches hard to review.
> Of course, in attachment.
Thanks ... but NAK on this. You removed the comments
explaining what the code is doing. Which makes the code
be less maintainable.
> >> Execution is more linear and without goto statement.
> >
> > YOu talk as if a well structured "goto" is a problem in itself.
> > That's not true ... but if that bothers you, all you had to
> > do was turn that into a "do { ... } while(updated)" loop.
> >
> > (And note that the reason for that additional loop was to handle
> > changes affecting the core loop criteria ... a quick read
> > didn't show you still addressing those issues.)
> In the original code the "for" loop iterates across the whole
> sectors[] array, while only the "first" and the "last" are really
> important.
Not true. For each pass, first and last matter ... but
when any given pass updates them, the *new* values will
matter for the next pass.
And if we assume (for discussion) that some needless looping
is happening ... it's going to be pretty minor: a fairly
tight loop, with code and data typially cache-resident.
I could *maybe* see one extra loop ... after the last one that
update the limits of the range. (There would be no more passes
made, as soon as one pass finds nothing to change.) And the
cost of such a loop will be small with respect to the I/O cost
of issuing a request to the flash.
> And every time "first" or "last" is changed, the array is scanned again.
Because when the range changes, some sectors that were previously not
examined might now need to be examined ... the range might need to
change yet again.
> Moreover, with original "for" boundaries, the condition "i == last" is
> never true inside loop's body.
True, but beside the point of my comments. You changed the continuation
test ... but not the body of the loop, which also tested "last".
> All such issues convinced me that code needs a deep review.
It's one tiny loop; no "deep" review possible. But as I pointed out,
your changes weren't well-enough reviewed.
> >> Moreover, it splits the first-last interval and should improve
> >> performance.
> >
> > I don't follow what you mean by this. Do you mean it addresses
> > the REVISIT comment by issuing multiple driver requests if it works
> > out that there are discontiguous regions? That's not necessarily
> > going to be faster... and such a change would have been more clearly
> > done as a separate patch.
>
> Yes, the idea is to include also the REVISIT comment in the final patch.
> But also remove all the useless iterations on the array
You mean "address" that comment? If so the patch comment should say
that's what's going on.
_______________________________________________
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development
