Hi Kristian,
Thanks for your comments. I sent out a few requests to the mailing list a
while back. I just took the silence as "whatever. it doesn't really affect
us." ... that being said, I actually share your concern about this
#ifdef-crud. My main reason for integrating it as-is was to avoid spending
days rebasing it the next time something big happens (like the directory
change). Unfortunately, my focus has been diverted elsewhere over the past few
months, so I wasn't able to clean this all up as I had hoped by now.
The main problem was that George forked from Mesa-7.2 and didn't preserve any
non-Apple code. It took me quite a while to re-integrate these changes to this
point, and hopefully things will get a bit smoother as I refactor it.
George decided to use a separate dispatch table for the gl functions, which has
served us in the short term, but now that I'm re-integrating these changes, I
want to rewrite our dispatch code to use the existing dispatch. This should
then let us eliminate more of these ifdefs and also start including support for
indirect.
I like the idea of using a similar approach for the glX functions, but for now,
I'm going to try making the GLX_USE_APPLEGL way more similar to the
!GLX_USE_APPLEGL way.
I'm sorry for any difficulty this has caused. Feel free to "break" Apple and
ping me to fix our case.
Thanks,
Jeremy
On May 20, 2010, at 06:24, Kristian Høgsberg wrote:
> Hi Jeremy,
>
> First of all, let me say that I'm sorry I didn't review your work
> before it was committed, that would have been the right time to
> address this. Anyway, my problem is that the addition of even more
> #ifdef's to the glx client code makes an already tangled code base
> harder to read and work with. Even worse, a lot of the new #ifndef
> GLX_USE_APPLEGL is of the form
>
> PUBLIC void
> glXCopyContext(Display * dpy, GLXContext source,
> GLXContext dest, unsigned long mask)
> {
> #ifdef GLX_USE_APPLEGL
> /* Apple GL code */
> #else
> /* What was there before */
> #endif
> }
>
> which is just screaming out for a vtable implementation. There are
> places where code is shared, but it's often just a small snippet like
> if (!dpy) return NULL; or it's a bigger piece of code that can be put
> in a common function and called from the different implementations.
>
> Of course, the existing code already did this to some extent with the
> direct/indirect code paths. Most of these cases look like
>
> #if defined(GLX_DIRECT_RENDERING)
> if (gc->driContext) {
> /* direct rendering case */
> }
> /* fall through to indirect case */
> #endif
>
> /* indirect case */
>
> which would be handled just fine by the vtable approach as well: at
> context creation time, we set the context vtable pointer to the
> direct, indirect or applegl functions and this will just work. I
> wanted to do some of this in the past, but I didn't feel like sinking
> too much work into the glx code before the license change.
>
> Now, I'm not saying that we need to do this refactoring now - it's
> certainly not a priority for me right now. But as a minimum, I'd like
> us to be on the same page with where this code should be, and
> hopefully we can work towards a cleaner code base. FIguring out
> what's going on in any of the glx entrypoints is just too hard right
> now with all the #ifdef spaghetti.
>
> Kristian
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
