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] 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 * src/glx/x11/glxclient.h remove structs marked as DEPRECATED * include/GL/internal/dri_interface.h DRI_NEW_INTERFACE_ONLY "survived" as "#if 0", better remove 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/ -------------------------------------------------------------------------------- -------------------------------------------------------------------------------- diff -Naur x11.orig/dri_glx.c x11/dri_glx.c --- x11.orig/dri_glx.c 2005-07-28 04:00:50.419361728 +0300 +++ x11/dri_glx.c 2005-07-28 03:56:09.851014000 +0300 @@ -41,7 +41,7 @@ #include "extutil.h" #include "glxclient.h" #include "xf86dri.h" -#include "sarea.h" +#include "GL/internal/sarea.h" #include <stdio.h> #include <dlfcn.h> #include "dri_glx.h" diff -Naur x11.orig/glcontextmodes.c x11/glcontextmodes.c --- x11.orig/glcontextmodes.c 2005-07-28 03:56:17.640830368 +0300 +++ x11/glcontextmodes.c 2005-07-28 03:44:37.297298000 +0300 @@ -35,7 +35,7 @@ # include <stdlib.h> # include <string.h> # include <GL/gl.h> -# include "dri_interface.h" +# include "GL/internal/dri_interface.h" # include "imports.h" # define __glXMemset memset #else diff -Naur x11.orig/glxext.c x11/glxext.c --- x11.orig/glxext.c 2005-07-28 02:32:47.655463912 +0300 +++ x11/glxext.c 2005-07-28 03:33:45.728352000 +0300 @@ -64,7 +64,7 @@ #include <inttypes.h> #include <sys/mman.h> #include "xf86dri.h" -#include "sarea.h" +#include "GL/internal/sarea.h" #include "dri_glx.h" #endif diff -Naur x11.orig/Makefile x11/Makefile --- x11.orig/Makefile 2005-07-28 02:32:47.671461480 +0300 +++ x11/Makefile 2005-07-28 03:47:35.191254000 +0300 @@ -5,21 +5,21 @@ # code will not build with DNIO defined. When we finally drop old interface # support in libGL, we need to clean up both glxcmds.c and dri_interface.h. -DEFINES += -DGLX_DIRECT_RENDERING -DGLXEXT -DXF86DRI -DGLX_USE_DLOPEN \ - -DGLX_USE_MESA -DXF86VIDMODE -D_REENTRANT -UIN_DRI_DRIVER +DEFINES += -DGLX_DIRECT_RENDERING -DXF86DRI -DXF86VIDMODE \ + -D_REENTRANT -UIN_DRI_DRIVER C_SOURCES = \ $(TOP)/src/mesa/glapi/glapi.c \ $(TOP)/src/mesa/glapi/glthread.c \ - glcontextmodes.c \ + $(TOP)/src/mesa/main/dispatch.c \ $(DRM_SOURCE_PATH)/libdrm/xf86drm.c \ $(DRM_SOURCE_PATH)/libdrm/xf86drmHash.c \ $(DRM_SOURCE_PATH)/libdrm/xf86drmRandom.c \ $(DRM_SOURCE_PATH)/libdrm/xf86drmSL.c \ clientattrib.c \ compsize.c \ - dispatch.c \ eval.c \ + glcontextmodes.c \ glxcmds.c \ glxext.c \ glxextensions.c \ @@ -54,15 +54,8 @@ INCLUDES = -I. \ -I$(TOP)/include \ - -I$(TOP)/include/GL/internal \ - -I$(TOP)/src/mesa \ -I$(TOP)/src/mesa/main \ -I$(TOP)/src/mesa/glapi \ - -I$(TOP)/src/mesa/math \ - -I$(TOP)/src/mesa/transform \ - -I$(TOP)/src/mesa/swrast \ - -I$(TOP)/src/mesa/swrast_setup \ - -I$(TOP)/src/mesa/drivers/dri/common \ -I$(DRM_SOURCE_PATH)/libdrm \ -I$(DRM_SOURCE_PATH)/shared-core \ $(X11_INCLUDES) @@ -80,12 +73,6 @@ default: depend $(LIB_DIR)/$(GL_LIB_NAME) -glcontextmodes.c: - ln -s $(TOP)/src/mesa/drivers/dri/common/glcontextmodes.c . - -dispatch.c: - ln -s $(TOP)/src/mesa/main/dispatch.c . - # Make libGL $(LIB_DIR)/$(GL_LIB_NAME): $(OBJECTS) Makefile $(TOP)/bin/mklib -o $(GL_LIB) -linker '$(CC)' \ @@ -93,10 +80,6 @@ -install $(LIB_DIR) $(GL_LIB_DEPS) $(OBJECTS) -drmtest: xf86drm.o drmtest.o - rm -f drmtest && $(CC) -o drmtest xf86drm.o drmtest.o - - depend: $(C_SOURCES) $(ASM_SOURCES) Makefile touch depend $(MKDEP) $(MKDEP_OPTIONS) $(INCLUDES) $(C_SOURCES) $(ASM_SOURCES)
(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.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.