[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/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.
>
Do you mean it was an intentional decision not to support allocation of
objects from the stack?  In some cases stack allocation is the most
efficient and least error-prone -- Like, in cases where the expected
lifetime of the object you're allocating is precisely the function or
the block you're allocating it from.

I'd vote for lifting this restriction.  In fact it seems you're already
allocating a number of ralloc-allocated objects from the stack in
several cases -- E.g. cfg_t, fs_live_variables, vec4_live_variables as
Paul pointed out.

>  * Use of rzalloc meant that objects would always be pre-allocated to zero.
>
The problem is that this moves part of the initialization semantics of
your classes into the memory allocator, which is problematical in cases
where you would like to use different allocation schemes with the same
object.  It's a common assumption that you're allowed to allocate
constructible types as you please, be it on the stack, within other
non-rzalloc'ed classes and structures, or with any variant of new, new()
or new[].

If we *really* want to break this assumption we should probably declare
all constructors private and use factory methods to enforce rzalloc as
the only usable allocator.

>  * 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.
>
nIt's the first thing any reasonably experienced C++ programmer would
think about when adding a new field to an existing class -- BTW, I
believe in many cases it's more convenient to use the 'foo()'
initialization syntax when you want to zero-initialize a large number of
member variables.

>  * 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. :)
>
You can also 'memset(0)' your class from its constructor if it's POD --
though it seems to me that manually initializing all fields to some
specific value is still the clearest way to communicate to the reader
what the intended initial value of some field was.

Thank you for your review.

> 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.
>[...]
-------------- 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/cbcf9ad0/attachment.pgp>