Thanks for review, Eric! On Thu, Oct 01, 2009 at 01:53:15PM -0700, Eric Anholt wrote: > On Thu, 2009-10-01 at 10:47 -0700, Jamey Sharp wrote: > > + drawables = xcalloc(screenInfo.numScreens, sizeof(DrawablePtr)); > > Seems like this drawable pointer should be part of the screen private.
That would be nice, but ProcPanoramiXShmGetImage seems to need an actual array. I didn't dig deeper to determine whether that array could be eliminated entirely, but I think a change like that would be more invasive than I'm quite prepared for. > > void > > ShmRegisterFuncs(ScreenPtr pScreen, ShmFuncsPtr funcs) > > { > > - shmFuncs[pScreen->myNum] = funcs; > > + ShmInitScreenPriv(pScreen)->shmFuncs = funcs; > > } > > I think this one might be a GetScreenPriv instead? I'm guessing that > ShmExtensionInit happens (set up protocol stuff), then later on DDXes > call ShmRegisterFuncs on their screen. That was my first guess too, but it seems to be the other way around. This was the crash at server startup in my first version of this patch. > > + ShmScrPrivateRec *priv; > > pScreen = screenInfo.screens[j]; > > > > - pMap = (*shmFuncs[j]->CreatePixmap)(pScreen, > > + priv = ShmGetScreenPriv(pScreen); > > Stylistically, I like private fetching to be part of the declaration. > Less chance for early dereferencing. Also, as many layers have screen > privates, pixmap privates, gc privates, etc., it can be nice to name the > variables for the privates screen_priv, pixmap_priv, etc. I'll fix these and the lack of a CloseScreen cleanup hook and send another patch. Thanks! Jamey
signature.asc
Description: Digital signature
_______________________________________________ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel