Re: [Aqsis-development] Review: refresh of SLO_* interface
Brought to you by:
ltatkinson,
pgregory
From: Malcolm H. <mal...@ma...> - 2010-04-03 11:40:58
|
> 1. It seems that you've turned the copyright header *from* C *to* C++, or have I mistaken something here. The copyright headers should now match between slo.h - slx.h and slo.cpp - slx.cpp, I tried to keep these files the same. > 2. Not quite sure why you've turned the Doxygen comment headers into single line standard headers. We do still generate Doxygen documentation, so having the Doxygen format headers for the slo stuff would be useful. I was removing the docs about the lookup table and thought as this is just wrapping the SLX_* interface to keep the comments a bit shorter. I have built the doc target and these comments do turn up docs unlike the slx.cpp ones. Let me know if you want me to add the @param tokens into these comments. btw I can only build the docs on my mac when I remove parser.yy and scanner.ll as doxygen just sits chewing up memory. > Otherwise, I agree with all other aspects of the change, including the switch to std::map for transferring data, makes a lot of sense. What are your thoughts on merging the functionality into the SLO_* code and retiring the SLX_* interface all together? .malcolm > Cheers > > > Paul > > On Sat, Apr 3, 2010 at 7:32 AM, Malcolm Humphreys <mal...@ma...> wrote: > Here is a new patch with the brackets, spaces and comments fixed. I think it's more important to keep all the files the same then change all of it in one go. > > Sorry I really should have given a bit more detail on the change just a bit lazy. > > * SLO_MATRIX is now 'typedef float*' and not a struct > * svd_name, svd_spacename and stringval in SLO_VISSYMDEF are now all 'const char *' > * added svd_valisvalid to SLO_VISSYMDEF > * added SLO_TYPE_SHADER to SLO_TYPE enum > * Slo_GetName now returns 'const char*' instead of 'char *' > * Added RSL2.0 functions to match prman and 3delights SLO_* interface > ** 'int Slo_HasMethod()' always returns 0. > ** 'const char* const* Slo_GetMethodNames()' always returns null. > * added an initialiser for the SLO_VISSYMDEF struct, to make sure we are always returning a valid struct. (initParamStruct()) > * replaced the SLX_VISSYMDEF lookup table code with a std::map as I found it hard to follow what was happening before. > * added a function to convert a SLX_VISSYMDEF to SLO_VISSYMDEF structure > > I would always use the SLO_* interface over the SLX_* as it doesn't require a heap of #ifdef AQSIS in the code to make it work. So I think in the long run it would be nice to just have the single SLO_* interface and remove the SLX_* as it seems silly to have nearly two identical interfaces for the same thing. > > Does anyone know if this is needed any more in slo.cpp as I don't have a windows machine to test it on > -- > #ifdef AQSIS_COMPILER_MSVC6 > #pragma warning (disable : 4786) > #endif //AQSIS_COMPILER_MSVC6 > -- > > .malcolm > > > > On 03/04/2010, at 1:42 PM, Chris Foster wrote: > > > On Sat, Apr 3, 2010 at 10:27 AM, Malcolm Humphreys > > <mal...@ma...> wrote: > >> Did you guys have any thoughts on this? > > > > Sorry for the silence Malcolm, I've just been busy (as you probably see, over > > at OIIO, among other things ;-) > > > > Generally speaking, I'm all for making the SLO interface as compatible as > > possible, that's what it exists for! If it's not compatible it's basically > > pointless having it at all. > > > > It looks like this patch contains a complete rework of the SLO implementation > > in a more C++ like style (thanks!). Would you be able to summarise the > > interface and implementation changes in a bit more detail? It's a bit hard to > > deduce the intent by reading wholesale changes via the diffs. > > > > > > Regarding the code, I've got a few initial comments (mostly aesthetic at this > > stage): > > > > - The aqsis codebase uses different brace placement than you've got here. I > > quite like the style you've used, but for the sake of consistency it would > > better to have all opening braces on their own line. > > > > - C++ style comments are banned in C-compatible headers. Realistically I'm not > > sure if there's any common compiler which doesn't support the // style, but > > for safety we go for the oldschool /* blah */ in slo.h > > > > - Spaces vs tabs. There was a recent discussion on IRC regarding changing over > > to spaces only from our long-time policy of tabs-only, so on balance I'm > > probably happy for you to use spaces as you've done. However, it does make > > the various files in libs/slxargs inconsistent. Does anyone else have > > objections to this? > > > > Cheers, and thanks for the patch, > > ~Chris > > > > ------------------------------------------------------------------------------ > > Download Intel® Parallel Studio Eval > > Try the new software tools for yourself. Speed compiling, find bugs > > proactively, and fine-tune applications for parallel performance. > > See why Intel Parallel Studio got high marks during beta. > > http://p.sf.net/sfu/intel-sw-dev > > _______________________________________________ > > Aqsis-development mailing list > > Aqs...@li... > > https://lists.sourceforge.net/lists/listinfo/aqsis-development > > > ------------------------------------------------------------------------------ > Download Intel® Parallel Studio Eval > Try the new software tools for yourself. Speed compiling, find bugs > proactively, and fine-tune applications for parallel performance. > See why Intel Parallel Studio got high marks during beta. > http://p.sf.net/sfu/intel-sw-dev > _______________________________________________ > Aqsis-development mailing list > Aqs...@li... > https://lists.sourceforge.net/lists/listinfo/aqsis-development > > > > > -- > Paul Gregory > http://www.aqsis.org > ------------------------------------------------------------------------------ > Download Intel® Parallel Studio Eval > Try the new software tools for yourself. Speed compiling, find bugs > proactively, and fine-tune applications for parallel performance. > See why Intel Parallel Studio got high marks during beta. > http://p.sf.net/sfu/intel-sw-dev_______________________________________________ > Aqsis-development mailing list > Aqs...@li... > https://lists.sourceforge.net/lists/listinfo/aqsis-development |