On Wed, 2016-09-14 at 06:30 -0400, Frediano Ziglio wrote:
> >
> >
> > I haven't looked very carefully at the changes, but a few comments
> > about readability:
> >
> > On Mon, Sep 12, 2016 at 04:32:03PM +0100, Frediano Ziglio wrote:
> > >
> > > This avoid to have the search the data scanning all the
> > > queue changing the order of these operations from O(n) to O(1).
> > >
> > > Signed-off-by: Frediano Ziglio <fzig...@redhat.com>
> > > ---
> > > server/dcc-send.c | 9 ++++-----
> > > server/dcc.c | 20 +++++++++-----------
> > > server/dcc.h | 2 +-
> > > server/red-channel-client.c | 32 +++++++++++++++++++++++++----
> > > ---
> > > server/red-channel-client.h | 4 +++-
> > > 5 files changed, 42 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/server/dcc.c b/server/dcc.c
> > > index 0d672df..ece37d9 100644
> > > --- a/server/dcc.c
> > > +++ b/server/dcc.c
> > > @@ -78,15 +77,14 @@ int
> > > dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc,
> > > int
> > > surface
> > > no other drawable depends on them */
> > >
> > > rcc = RED_CHANNEL_CLIENT(dcc);
> > > - l = rcc->priv->pipe.head;
> > > - while (l != NULL) {
> > > - GList *cur = l;
> > > + for (l = rcc->priv->pipe.head; l != NULL; item_pos = NULL) {
> >
> > Please no :) the while() version might be better. The for()
> > version
> > with the loop index being updated within the loop, but only after
> > doing
> > a bit of work with the previous value of the loop index is really
> > unexpected.
> >
>
> What can I say: I give up!
Give up? Why?
>
> This patch is not related to GQueue nor to optimization.
> This fixes a regression introduced in the first patch.
Which patch was the regression introduced in? Was that patch merged
already?
> I spotted this regression the first time (some months ago) this
> patch was posted on the mailing list but no matter saying that some
> path touched some sensible code that required more attention, no
> matter all my consideration about complexity no one pick up again
> these patches and noted the issue.
I'm afraid that I don't remember a discussion from some months ago. If
there was a regression introduced by a patch that has already been
merged, then you should feel free to ping for review if something was
not getting reviewed. I wasn't aware of any regressions.
On the other hand, if the regression was introduced in one of the
patches that are still under review, then I don't quite understand your
frustration. Isn't finding and fixing regressions part of what a review
is for?
>
> Now I could think different causes:
> - we are too tired of these patches and we don't pay much attention;
> - we look more at style than behavior;
> - we are too busy to spend too much time on the review.
> Any of the reason do not seem really a good reason to me.
> Any other reason I don't see?
>
Do you have suggestions for improving things?
Jonathon
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
