Bug 3899

(In reply to comment #0)
> In the spirit of cleaning libGL code, here are some minor suggestions:
> 
> *	src/mesa/drivers/dri/common/glcontextmodes.c
> 
> 	__DRIinterfaceMethodsRec provides both createContextModes and 
> 	destroyContextModes. The dri drivers use createContextModes from 
> 	__DRIinterfaceMethodsRec and _gl_context_modes_destroy directly 
> 	from glcontextmodes.c (called within dri_util.c). Is this on purpose ? 
> 	Otherwise, the dri drivers could use destroyContextModes instead of 
> 	_gl_context_modes_destroy. In that case, it is possible to:
> 
> 	o	move glcontextmodes.[hc] to src/glx/x11/
> 	o	eliminate IN_DRI_DRIVER from glcontextmodes.[hc]

That should be doable.  Having glcontextmodes.c compiled into the DRI drivers is
an artifact of having to support versions of libGL that didn't supply it.  Since
we no longer need to support those versions, it can go.

> 	Moreover, 
> 
> 	o	functions _gl_convert_to_x_visual_type and 
> 		_gl_copy_visual_to_context_mode appear to be dead code
> 	o	the #ifdef XFree86Server directive appears to be dead code 
> 		since the symlink-mesa.sh script from xorg does not symlink 
> 		glcontextmodes.c

Where does it get glcontextmodes.c?  I set it up like that so that we wouldn't
have to keep two copies of the file in sync.

> *	src/glx/x11/glxclient.h
> 
> 	remove structs marked as DEPRECATED

Yes!  Those can finally go away!  They had to be left in place because changing,
moving, or removing them broke binary compatibility with older DRI drivers.  A
few of the changes in the "new" interface were designed specifically to fix
this.  We should be able to gut these at any point with no problems.

> *	include/GL/internal/dri_interface.h
> 
> 	DRI_NEW_INTERFACE_ONLY "survived" as "#if 0", better remove it

Oops.  I changed it to '#if 0' to make sure it really could be removed, but
never went back and removed it.

> *	src/glx/x11/Makefile
> 
> 	see patch below (tested with glxgears), under the assumptions that:
> 
> 	o	glcontextmodes.[hc] was treated as above
> 	o	dispatch.c can be compiled similar to glapi.c, not symlinked
> 	o	the code in src/mesa/main/glheader.h within 
> 		the #ifndef __MINGW32__ directive is actually 
> 		moved somewhere in src/mesa/drivers/windows/

I think that should be doable.  I'll have to look at it a bit more.

FYI, in-line patches are pure evil. :)

(In reply to comment #1)
> > 	Moreover, 
> > 
> > 	o	functions _gl_convert_to_x_visual_type and 
> > 		_gl_copy_visual_to_context_mode appear to be dead code
> > 	o	the #ifdef XFree86Server directive appears to be dead code 
> > 		since the symlink-mesa.sh script from xorg does not symlink 
> > 		glcontextmodes.c
> 
> Where does it get glcontextmodes.c?  I set it up like that so that we wouldn't
> have to keep two copies of the file in sync.
> 
there is a checked in copy at http://cvs.freedesktop.org/xorg/xserver/xorg/GL/glx/


> > *	src/glx/x11/Makefile
> > 
> > 	see patch below (tested with glxgears), under the assumptions that:
> > 
> > 	o	glcontextmodes.[hc] was treated as above
> > 	o	dispatch.c can be compiled similar to glapi.c, not symlinked
> > 	o	the code in src/mesa/main/glheader.h within 
> > 		the #ifndef __MINGW32__ directive is actually 
> > 		moved somewhere in src/mesa/drivers/windows/
> 
> I think that should be doable.  I'll have to look at it a bit more.
> 
that was before IN_DRI_DRIVER was used in dispatch.h
in the discussion for mesa-solo you said that the drivers don"t actually link
dispatch.c. Is that all drivers ?
then dispatch.c should be moved to mesa/glapi, 
this would fix the mess with building dispatch.c in three different ways with
specific order each time and make libGL code self-contained (only use mesa/glapi).

i will provide some more details and an attached patch if this the case.

Here is the suggestion in detail:

*	move glcontextmodes.[hc] to glx/x11
*	move dispatch.c to mesa/glapi
*	kill glx/mini/dispatch.c
*	apply the attached patch (mesa/sources, glx/*/Makefile)

glcontextmodes.c is no longer linked in DRI drivers

dispatch.c is compiled in the following ways:

*	non-dri   :	in-place from mesa/Makefile
*	linux-dri :	in-place from mesa/Makefile (unused)
			symlink  from glx/x11/Makefile
*	linux-solo:	in-place from glx/mini/Makefile
			in-place from mesa/Makefile (does nothing)
			in this case module compilation order is 
			important: glx/mini before mesa

This can be fixed by moving dispatch.c to mesa/glapi,
mesa/Makefile needs no changes: in the DRI case SOLO_SOURCES 
is used which does not contain GLAPI_SOURCES but DRI drivers 
don't mind and most importantly they *don't touch* dispatch.c; 
in the non-DRI case ALL_SOURCES is used which contains both 
MAIN_SOURCES and GLAPI_SOURCES, (except for beos which gets 
GLAPI_SOURCES in its own Makefile)

Created attachment 3311 [details] [review]
Patch to implement the changes described above.

I think all of the suggested changes that do not require moving files have been
committed.  Moving glcontextmodes.[ch] and dispatch.c is a good idea, but I
don't want to create extra havoc importing updates of Mesa that move files until
after X.org 6.9 ships.  Once that ships, we'll never have to import Mesa into
the X.org tree again (yay!), so this won't be an issue.

(In reply to comment #5)
> I think all of the suggested changes that do not require moving files have been
> committed.  Moving glcontextmodes.[ch] and dispatch.c is a good idea, but I
> don't want to create extra havoc importing updates of Mesa that move files until
> after X.org 6.9 ships.  Once that ships, we'll never have to import Mesa into
> the X.org tree again (yay!), so this won't be an issue.

Isn't it time to close this ?

Also, what about the following aesthetic renames during the cvs surgery ?
glx_texture_compression.c	-> indirect_texture_compression.c
indirect_va_private.h		-> indirect_vertex_array_priv.h

All of the cleanups that were going to be made have been made.