[Mesa-dev] [PATCH 02/24] glsl: Initialize all member variables of _mesa_glsl_parse_state on construction.

[Francisco Jerez]

Ian Romanick <idr at freedesktop.org> writes:

> On 09/18/2013 01:35 PM, Paul Berry wrote:
>>[...]
>> * Initializing class members in the constructor is common practice in
>> C++, and therefore less surprising to developers who are new to the Mesa
>> code base.
>> 
>> * If a class initializes all its members in the constructor, we have the
>> freedom to allocate it in whatever way is appropriate (stack, rzalloc,
>> malloc, new operator, or placement new, to name a few examples).  If it
>> doesn't, we have no choice but to allocate it using rzalloc.
>> 
>> * There's no compile-time mechanism to ensure that all uses of a class
>> use rzalloc (nor are there any static analysis tools I'm aware of that
>> are capable of ensuring this).  It's also a pain to verify using grep. 
>> So if a class doesn't initialize all its members in the constructor, we
>> have to be vigilant in our code review to make sure that all present and
>> future users of the class use rzalloc.
>
> As far as I'm aware, our use of overloaded new / delete with rzalloc
> forces us to use new (and therefore rzalloc) for every object or die in
> a fire.  None of the destructors deallocate any objects owned by them.
> Even if we did switch to a different allocation scheme, we would leak
> like a screen door on a submarine.
>
But, we're already using other allocation schemes -- and that's fine,
they are useful.  I think those leaks and unpredictable results we get
when using allocators other than rzalloc is something that it would be
worthwhile to fix, having all constructors initialize all class members
is a first step, even if we decide to keep using memset(0) to that
end...

>> * By contrast, Coverity *is* able to ensure that class members are
>> initialized in the constructor, and I suspect other static analysis
>> tools are too.
>
> Relying on Vinson to run Coverity and submit bugs is not a solid plan.
> We have no other plan involving static analysis tools, and we don't have
> the staff to filter the flood of false positives that ever static
> analysis tool I've ever encountered generates.

How about making operator new poison the allocated memory with some
easily recognizable pattern in debug builds to keep future problems of
this kind from going unnoticed when a new member variable is introduced?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130918/82648a30/attachment.pgp>