[Mesa-dev] [PATCH 3/4] glsl: add ir_cache class and functions for shader serialization

[Eric Anholt]

Tapani Pälli <tapani.palli at intel.com> writes:

> On 10/29/2013 03:48 PM, Paul Berry wrote:
>> On 29 October 2013 00:53, Tapani <tapani.palli at intel.com 
>> <mailto:tapani.palli at intel.com>> wrote:
>>>         +}
>>>         +
>>>         +
>>>         +/* data required to serialize ir_variable */
>>>         +struct ir_cache_variable_data
>>>
>>>
>>>     As with glsl_type_data, I don't understand why we need to have
>>>     this class rather than just adding a serialize() method to the
>>>     ir_variable class.
>>
>>     This could be a serialize function in ir_cache_serialize.cpp. I
>>     did not want to touch existing ir classes, I think it helps to
>>     have the cache code separated and in one place. This is of course
>>     debatable, every ir instruction could have serialize() and
>>     unserialize() to deal with this.
>>
>>
>> My chief worry with separating them is that people will forget to 
>> update the serialization code when they change the ir class, because 
>> the thing they need to update will be far away.  This is particularly 
>> a problem when adding new members to the class (something we do quite 
>> frequently when implementing new GLSL features).
>
> I understand and I've been worried about this from the maintainability 
> point of view as well. I've thought to add some documentation on the 
> ir.h file about this.

If it's not tested, it will break, and documentation will only help you
say "I told you so!", which is not very useful.  This is why we've been
emphasizing automatic caching over get_program_binary -- because
everyone (except crazy RO root systems) will always be using it,
developers will actually catch the bugs.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131101/161bbb41/attachment.pgp>