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

[Ian Romanick]

On 09/18/2013 01:35 PM, Paul Berry wrote:
> On 18 September 2013 07:43, Ian Romanick <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> wrote:
> 
>     On 09/15/2013 02:10 AM, Francisco Jerez wrote:
>     > The _mesa_glsl_parse_state object relies on the memory allocator
>     > zeroing out its contents before it's initialized, which seems rather
>     > risky.  One of the following commits will homogenize implementations
>     > of the new operator in a way that would break this assumption leaving
>     > some of the member variables of this object uninitialized.
> 
>     This was an intentional design decision that we made, after a fair
>     amount of consideration, in the early days of the compiler.  A number of
>     factors came together:
> 
>      * Use of ralloc et al. meant that we could never have stack-allocated
>     objects.
> 
>      * Use of rzalloc meant that objects would always be pre-allocated
>     to zero.
> 
>      * Uninitialized class member bugs are hard to track down.
> 
>      * It's easy to forget to add 'this->foo = 0;' when you add a field foo
>     to a class.
> 
>      * A dozen lines of 'this->foo = 0;' is hard to properly review when a
>     new class is added.  Are there actually 13 fields that need to be
>     initialized?
> 
>      * Writing a dozen lines of 'this->foo = 0;' is annoying. :)
> 
>     In the end, we decided to just rely on the allocator always providing
>     cleared memory.
> 
>     I'm not very excited about adding a bunch of redundant code to the these
>     constructors... especially since that code probably won't get maintained
>     (e.g., when the next member gets added to those classes).
> 
>     I'd like to hear Ken and Eric weigh in.
> 
> 
> I think I have sufficient development experience and familiarity with
> the code base to weigh in as well.  I fall on the opposite side of this
> particular fence--I prefer to see class members initialized in the
> constructor.  My reasoning is:
> 
> * Initializing class members in the constructor costs almost nothing and
> decreases the risk of subtle and difficult-to-find bugs.

Except that we forget to add the initialization almost as often as we
remember.  I see from the patch that some of the fields lacking
initializers are ones you most likely added... :)

> * 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.

> * 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.