On Wed, Sep 14, 2016 at 06:30:42AM -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 <fziglio@xxxxxxxxxx> > > > --- > > > 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! > > This patch is not related to GQueue nor to optimization. > This fixes a regression introduced in the first patch. If the regression which is fixed is *not* a performance/complexity regression, does it belong to that patch, or could it be moved to a different one? Note that I'm not saying "don't fix the regression", just saying that this for loop is far too magic, hopefully it's possible to fix it differently? > 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. > > 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? I think I just missed your initial comments/patches, feel free to ping again when important patches are getting missed. Also, if this patch fixes a non-performance bug, an explanation would be useful to ease review/get more focused review :) Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel