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

[Ian Romanick]

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.

> ---
>  src/glsl/glsl_parser_extras.cpp | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/src/glsl/glsl_parser_extras.cpp b/src/glsl/glsl_parser_extras.cpp
> index cac5a18..772933f 100644
> --- a/src/glsl/glsl_parser_extras.cpp
> +++ b/src/glsl/glsl_parser_extras.cpp
> @@ -55,7 +55,7 @@ static unsigned known_desktop_glsl_versions[] =
>  
>  _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct gl_context *_ctx,
>  					       GLenum target, void *mem_ctx)
> - : ctx(_ctx)
> +   : ctx(_ctx), switch_state()
>  {
>     switch (target) {
>     case GL_VERTEX_SHADER:   this->target = vertex_shader; break;
> @@ -66,10 +66,14 @@ _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct gl_context *_ctx,
>     this->scanner = NULL;
>     this->translation_unit.make_empty();
>     this->symbols = new(mem_ctx) glsl_symbol_table;
> +
> +   this->num_uniform_blocks = 0;
> +   this->uniform_block_array_size = 0;
> +   this->uniform_blocks = NULL;
> +
>     this->info_log = ralloc_strdup(mem_ctx, "");
>     this->error = false;
>     this->loop_nesting_ast = NULL;
> -   this->switch_state.switch_nesting_ast = NULL;
>  
>     this->struct_specifier_depth = 0;
>     this->num_builtins_to_link = 0;
> @@ -105,6 +109,13 @@ _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct gl_context *_ctx,
>  
>     this->Const.MaxDrawBuffers = ctx->Const.MaxDrawBuffers;
>  
> +   this->current_function = NULL;
> +   this->toplevel_ir = NULL;
> +   this->found_return = false;
> +   this->all_invariant = false;
> +   this->user_structures = NULL;
> +   this->num_user_structures = 0;
> +
>     /* Populate the list of supported GLSL versions */
>     /* FINISHME: Once the OpenGL 3.0 'forward compatible' context or
>      * the OpenGL 3.2 Core context is supported, this logic will need
> @@ -163,6 +174,7 @@ _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct gl_context *_ctx,
>  
>     this->gs_input_prim_type_specified = false;
>     this->gs_input_prim_type = GL_POINTS;
> +   this->gs_input_size = 0;
>     this->out_qualifier = new(this) ast_type_qualifier();
>  }
>  
>