[PATCH 0/37] Extension handling cleanup, death to extmod

[Jamey Sharp]

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 <jamey at minilop.net>

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.x.org/archives/xorg-devel/attachments/20110630/ad5cb990/attachment.pgp>