Chia-I, I'm still reviewing your patches (but it's getting late and I need to stop for the day). I think some of what you've done is good, but other parts I'm not convinced we need. Here's some assorted comments.
> Subject: [PATCH 02/20] mesa/main: Make FEATURE_pixel_transfer follow feature > conventions. > > As shown in mfeatures.h, this allows users of pixel.h to work without > knowing if the feature is available. It also renames > FEATURE_pixel_transfer to FEATURE_pixel. Please don't rename this to FEATURE_pixel. I want to keep a strong distinction between "pixel transfer" and "pixel store" functions. ---------------------------------------------------------------------- > Subject: [PATCH 14/20] mesa/main: Make FEATURE_evaluators follow feature > conventions. > > As shown in mfeatures.h, this allows users of eval.h to work without > knowing if the feature is available. It also renames FEATURE_evaluators > to FEATURE_eval. Again, I'd prefer that you not rename FEATURE_evaluators. IIRC, I named the file eval.c because back in the 8.3 days evaluators.c would have been too long. ---------------------------------------------------------------------- Using patch 0003 as an example... Note that if you use _mesa_init_colortable_dispatch() to plug in all the colortable functions into the dispatch table, I think that all the _mesa_ColorTable* functions can be made static. That would be a nice improvement. In many other cases, the only other calls to the _mesa_FooBar() state- setters is from the glPopAttrib() code. Maybe that could be changed somehow to allow more static functions. ---------------------------------------------------------------------- > I wanted to make #if's more a one-time effort. The #if's are either in > convolve.h, or in every location, say, > _mesa_adjust_image_for_convolution is called. This includes in st, > meta, and drivers. It becomes horrible soon. The _mesa_adjust_image_for_convolution() function could easily be made a no-op function so we don't have to #ifdef it everywhere. That's an easy one to fix. ---------------------------------------------------------------------- Patches 0017/0018/0019/0020: I think I'd prefer to keep the texformat code as-is for now. ---------------------------------------------------------------------- Dave wrote: > does the current feature use ifdefs in the C code to avoid calling > entrypoints if the feature isn't enabled? vs having null C functions in > the header files? If a function isn't plugged into the dispatch table, that slot in the dispatch table will point to the default/nop dispatch function. We won't try to jump through a NULL function pointer. > The first isn't generally a good idea as code doesn't get built when > the feature is off, so people editing around the feature can forget to > fix things inside the ifdefs, the second approach is a lot sounder > generally. Normally all featueres are on. If you're adding a new feature and forget to provide and plug in an API function, you'll quickly know about it regardless of which method is used (presuming you test your code). ---------------------------------------------------------------------- Chia wrote: > There is also one new macro for driver writers. Right now, driver > functions are initialized by > > driver->Blah1 = implBlah1; > driver->Blah2 = implBlah2; > > The patch series encourages drivers to switch to > > _MESA_INIT_BLAH_FUNCTIONS(driver, impl); I guess I'm not too enthusiastic about this change in particular. It's a clunky macro and the current method is much simpler. It also implies changing all the drivers. Nobody has ever had a problem with this in the past so I think it's a non-issue. ---------------------------------------------------------------------- If you agree with this feedback, would you mind re-spinning the first few patches? I'd rather focus on getting a few things (like maybe colortable and convolution) sorted out as examples before we spend too much time on the rest. Thanks. -Brian ------------------------------------------------------------------------------ Come build with us! The BlackBerry® Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9-12, 2009. Register now! http://p.sf.net/sfu/devconf _______________________________________________ Mesa3d-dev mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/mesa3d-dev
