Hi Daniel! I sure like the results of this series. I've reviewed the patches as I found them on your personal branch. The abbreviated commit hashes that I reviewed are listed together with their commit summaries below.
Suggestion for "Replace INITARGS with void": This commit message is a little misleading because you already fixed Xinerama's prototype inconsistency in the previous patch. In "Move Xv and XvMC from extmod": why does sdksyms.sh no longer need to pass -DXorgLoader at this point? And should that be a separate commit, maybe part of removing extmod? For the following patches, you can add my: Reviewed-by: Jamey Sharp <[email protected]> 6dd551a Add a common ARRAY_SIZE macro to dix.h b15983c Make extension.h self-contained, remove C++ externs 784b193 Xinerama: Fix ExtensionInit prototype e7a0286 Replace INITARGS with void b065058 Move DBE from an external module to built-in e0acea8 Move RECORD from external module to built-in d1645b6 Move MIT-SCREEN-SAVER from extmod to built-in 1cca9e4 Move DPMS from extmod to built-in 3ec519a Move XRes from extmod to built-in 3d53063 Move Xv and XvMC from extmod to built-in b3b40de Move SELinux from extmod to built-in b9723f4 DRI2: Remove prototype for DRI2DestroyDrawable dc3ca29 Move DRI2 from external module to built-in f47c514 Loader: Move ExtensionModule types to DIX ce0cc09 Move the remnants of loadext.c to miinitext.c 78fff98 Remove unused setupFunc from extensions f2e8c2e Add localOnly argument to AddExtension 0629243 Hide local-only extensions from remote clients I think these patches need work: 6856901 Add xf86ExtensionInit for DDX extension configuration - Good plan; I suggest squashing part of "Move XFree86-VidMode from extmod" into this patch though. Details in the thread for that patch. - Also, xf86ExtensionInit initializes modp to the same value twice at the beginning of the loop. I'd suggest leaving the first initialization and delete the second one, making the loop header one line. 9601c02 Move DGA from extmod to built-in da1bfbf Move XFree86-VidMode from extmod to built-in 0b40ab8 Remove the last remnants of extmod - Some re-ordering needed; details in the thread for "Move XFree86-VidMode from extmod". - Anyway, I'd be tempted to leave these two extensions in extmod. All the other de-modularizings make sense because they're trivial module wrappers around code that was linked into the server core anyway. These two aren't. - I'd rather see all the *ExtensionInit functions have real prototypes in some header(s), instead of copying prototypes all over. The easy way to start on that would be to move modinit.h to Xext/ or include/ or something, and just delete everything except the Init function prototypes. That makes sense as a standalone patch and makes "Remove the last remnants of extmod" much smaller. 58162d2 Xext: Only build one library - This looks great, except that I think deleting this line: INCLUDES = -I$(top_srcdir)/hw/xfree86/dixmods/extmod needs to happen in whichever patch moves/deletes modinit.h. With that addressed, feel free to add my reviewed-by. 083f0ec Move DRI1 from external module to built-in - I think this lost something; see the thread for this patch. 5396316 GLX: Remove extension init dependencies - This doesn't look right, off-hand. My reading of the code is that extensions will be initialized in the order that LoadExtension was called, and that static extensions will have LoadExtension called from InitExtensions *after* all modules are loaded. So GLX would init before its dependencies, right? b1a6636 Loader: Remove extension initialisation sorting 65ce43d Remove initDependencies from ExtensionModule - Of course you can't remove the sorting without removing its user, but assuming that gets, er, sorted, these patches make me happy. I'd squash them together, though, so that the compiler can catch anything still trying to use initDependencies-based sorting. 4bd55bc Ignore local-only requests from remote clients - See the thread for this patch. e2ea4a6 Remove separate ExtensionToggle list from miinitext [WIP] - Duh, it's WIP. :-) But I think it's easy to fix: Either do your lookups in both staticExtensions and ExtensionModuleList, or arrange to LoadExtension for all staticExtensions before calling anything else in miinitext.c, and then use only ExtensionModuleList. And I'm too lazy to think about these. :-) 1406700 Reorder extension initialisation for non-Xorg 82b33bf Remove Xorg-specific extensions from non-Xorg miinitext 71fce67 Remove LocalClient checks from local-only extensions - I'm not prepared to review these. 07cd310 Loader: Drop EXTERN_MODULE flag - I don't feel like checking the assertion that the flag is unused. 21ce582 Xv: Remove excessive module-induced indirection 0c6b14f DGA: Remove excessive module-induced indirection 5877a46 Unify miinitext.c - Great ideas, but I don't wanna reason through whether the patches are correct. 62aaaae XFree86: DRI: Don't use per-target CFLAGS - I trust Cyril's review for this, but not mine. Jamey
signature.asc
Description: Digital signature
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
