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

[Kenneth Graunke]

On 09/18/2013 07:43 AM, Ian Romanick 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 do like simply using rzalloc or memset to avoid having to fill in
dozens of fields with 0.  It's very concise and convenient.

However, an object's constructor is supposed to initialize its fields.
This is idiomatic for *any* object oriented language.  If we're going to
use the OOP paradigm, we probably ought take this approach.

In my mind, using memset() inside the constructor counts as the
constructor initializing the fields.  It also works with stack
allocation, normal heap allocation, or ralloc allocation.

Uninitialized class member bugs will be caught with Valgrind, and Piglit
makes it very easy to check that:

$ piglit-run.py --valgrind tests/quick.tests results/vg-1

With these tools, I don't think tracking down uninitialized fields is
that difficult.  I also don't think "we'll forget to initialize things"
is a good argument for relying on zero-allocation.  Sometimes fields
need to be initialized to a value other than zero, so it's a good idea
to get in the habit of initializing them.

I would have to say I agree with Paul and Curro, but I can also respect
the current rely-on-rzalloc style, and it doesn't honestly bother me
much.  If people want to explicitly initialize member variables, I
applaud them; if not, I can live with that too.

--Ken