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

[Paul Berry]

On 18 September 2013 07:43, Ian Romanick <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.

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

* By contrast, Coverity *is* able to ensure that class members are
initialized in the constructor, and I suspect other static analysis tools
are too.


>
> > ---
> >  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();
> >  }
> >
> >
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130918/025faaf4/attachment-0001.html>