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

[Kenneth Graunke]

On 09/20/2013 09:51 PM, Francisco Jerez wrote:
> The _mesa_glsl_parse_state object relies on the memory allocator
> zeroing out its contents before it's initialized, which is quite an
> unusual practice in the C++ world because it ties objects to some
> specific allocation scheme, and gives unpredictable results when an
> object is created with a different allocator -- Stack allocation,
> array allocation, or aggregation inside a different object are some of
> the useful possibilities that come to my mind.  Initialize all fields
> from the constructor and stop using the zeroing allocator.
> 
> Reviewed-by: Paul Berry <stereotype441 at gmail.com>
> Reviewed-by: Chad Versace <chad.versace at linux.intel.com>
> ---
>  src/glsl/glsl_parser_extras.cpp | 16 ++++++++++++++--
>  src/glsl/glsl_parser_extras.h   |  2 +-
>  2 files changed, 15 insertions(+), 3 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()

Maybe it's just me, but I don't particularly like using explicit calls
to undefined constructors to zero out data.

It's a bit of an inconsistency in C++: for most classes, the default
constructor does nothing.  But for POD, the default constructor
zero-initializes the data.  Unless you're familiar with this rule, it
looks like a no-op.

Perhaps more importantly, the moment someone adds a method to one of
these classes, the default constructor's behavior changes from
zero-initializing to no-op.  It's easy to forget that, and suddenly have
a bunch of uninitialized data.

I would prefer to see explicit constructors for POD types which
zero-initialize all of the fields.  Then, you don't need to call the
constructor - it just happens automatically.  If you add methods, it
keeps working.

For tiny structures, like two or three elements, I'd also be okay with
the containing class's constructor initializing them.  For example,
_mesa_glsl_parse_state() could just initialize switch_state->test_var
and so on.  (In this case I think glsl_switch_state is too large for
this approach.)

Sorry for the nitpicking.  Otherwise, I'm definitely in favor of this
change.

>  {
>     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();
>  }
>  
> diff --git a/src/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h
> index 364a983..853a9b0 100644
> --- a/src/glsl/glsl_parser_extras.h
> +++ b/src/glsl/glsl_parser_extras.h
> @@ -73,7 +73,7 @@ struct _mesa_glsl_parse_state {
>     _mesa_glsl_parse_state(struct gl_context *_ctx, GLenum target,
>  			  void *mem_ctx);
>  
> -   DECLARE_RZALLOC_CXX_OPERATORS(_mesa_glsl_parse_state);
> +   DECLARE_RALLOC_CXX_OPERATORS(_mesa_glsl_parse_state);
>  
>     /**
>      * Generate a string representing the GLSL version currently being compiled
>