On Tue, Sep 22, 2009 at 09:28:22PM -0600, Brian Paul wrote: > 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. Thanks for the reviewing. I will reply inline. > > 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. They are renamed to keep in sync with their init functions, not the file names. That is, their init functions are named _mesa_init_pixel and _mesa_init_eval respectively. Should I also rename the init functions while undo the renames? > 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. Will do. > 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 think I will make _mesa_FooBar functions that are not used globally static first. They should include most of functions.
Other than glPopAttrib, some of the remaining ones are used in meta.c. > > 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. That is one of the goals this patch tries to do :) > ---------------------------------------------------------------------- > Patches 0017/0018/0019/0020: > I think I'd prefer to keep the texformat code as-is for now. Hmm. To make intel dri driver compile with the compressed texformats disabled, I have to add like 10 #if/#endif pairs to leave out only a tiny fraction of the code. What do you think if I still make disabled texformats become no-op, but remove all other changes from the patches? > > 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. Talking about dispatching, for those entry points that are disabled by a feature, they will be generic_nop, the default value. If the entry points are also found in GLvertexformat, they will be NULL, and will never to be installed into the dispatch table. If some driver functions in dd_function_table are needed to implement the feature, they will be NULL too. This is minor. But it might not be obvious on first sight of the patches. > > 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). > > 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. It is sort of a helper macro/replacement for the existing st_init_blah_functions or intelInitBlahFunctions. I do not intend to convert existing drivers to use them. But I would like to see them used when new features are added. I think the awareness of something is a feature from a driver author is important and very helpful. This should be easier to achieve if there is a convention to follow. When new extensions are supported, it is not uncommon that the core mesa part is implemented as a feature, while the driver part isn't. IMHO, this is because there is an established convention in core mesa, but there isn't one in drivers. I would like to keep the macro if possible. But, yeah, driver authors would need to adopt it for it to be useful. So it would be even better if there is another form that is acceptable by most people. > 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. I will be working on it. Hopefully in time for today. -- Regards, olv ------------------------------------------------------------------------------ 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 Mesa3d-dev@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mesa3d-dev
