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

[Paul Berry]

On 18 September 2013 13:15, Ian Romanick <idr at freedesktop.org> wrote:

> On 09/18/2013 01:35 PM, Paul Berry wrote:
> > On 18 September 2013 07:43, Ian Romanick <idr at freedesktop.org
> > <mailto: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.
>
> Except that we forget to add the initialization almost as often as we
> remember.  I see from the patch that some of the fields lacking
> initializers are ones you most likely added... :)
>

Only because when I added those fields I was following the prevailing
convention.


>
> > * 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.
>
> As far as I'm aware, our use of overloaded new / delete with rzalloc
> forces us to use new (and therefore rzalloc) for every object or die in
> a fire.  None of the destructors deallocate any objects owned by them.
> Even if we did switch to a different allocation scheme, we would leak
> like a screen door on a submarine.
>
> > * By contrast, Coverity *is* able to ensure that class members are
> > initialized in the constructor, and I suspect other static analysis
> > tools are too.
>
> Relying on Vinson to run Coverity and submit bugs is not a solid plan.
> We have no other plan involving static analysis tools, and we don't have
> the staff to filter the flood of false positives that ever static
> analysis tool I've ever encountered generates.
>
>
Actually, I believe Vinson just uses the results of the scans that Coverity
runs on open source projects such as Mesa for free.  Anyone can view the
open defects here: https://scan.coverity.com/projects/139, and anyone can
sign up to receive e-mail notifications of new defects.  I'm signed up to
receive the emails too.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130918/c5d1b512/attachment-0001.html>