<div dir="ltr">On 18 September 2013 13:15, Ian Romanick <span dir="ltr"><<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>On 09/18/2013 01:35 PM, Paul Berry wrote:<br>
> On 18 September 2013 07:43, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a><br>
</div><div><div>> <mailto:<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>>> wrote:<br>
><br>
> On 09/15/2013 02:10 AM, Francisco Jerez wrote:<br>
> > The _mesa_glsl_parse_state object relies on the memory allocator<br>
> > zeroing out its contents before it's initialized, which seems rather<br>
> > risky. One of the following commits will homogenize implementations<br>
> > of the new operator in a way that would break this assumption leaving<br>
> > some of the member variables of this object uninitialized.<br>
><br>
> This was an intentional design decision that we made, after a fair<br>
> amount of consideration, in the early days of the compiler. A number of<br>
> factors came together:<br>
><br>
> * Use of ralloc et al. meant that we could never have stack-allocated<br>
> objects.<br>
><br>
> * Use of rzalloc meant that objects would always be pre-allocated<br>
> to zero.<br>
><br>
> * Uninitialized class member bugs are hard to track down.<br>
><br>
> * It's easy to forget to add 'this->foo = 0;' when you add a field foo<br>
> to a class.<br>
><br>
> * A dozen lines of 'this->foo = 0;' is hard to properly review when a<br>
> new class is added. Are there actually 13 fields that need to be<br>
> initialized?<br>
><br>
> * Writing a dozen lines of 'this->foo = 0;' is annoying. :)<br>
><br>
> In the end, we decided to just rely on the allocator always providing<br>
> cleared memory.<br>
><br>
> I'm not very excited about adding a bunch of redundant code to the these<br>
> constructors... especially since that code probably won't get maintained<br>
> (e.g., when the next member gets added to those classes).<br>
><br>
> I'd like to hear Ken and Eric weigh in.<br>
><br>
><br>
> I think I have sufficient development experience and familiarity with<br>
> the code base to weigh in as well. I fall on the opposite side of this<br>
> particular fence--I prefer to see class members initialized in the<br>
> constructor. My reasoning is:<br>
><br>
> * Initializing class members in the constructor costs almost nothing and<br>
> decreases the risk of subtle and difficult-to-find bugs.<br>
<br>
</div></div>Except that we forget to add the initialization almost as often as we<br>
remember. I see from the patch that some of the fields lacking<br>
initializers are ones you most likely added... :)<br></blockquote><div><br></div><div>Only because when I added those fields I was following the prevailing convention.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><br>
> * Initializing class members in the constructor is common practice in<br>
> C++, and therefore less surprising to developers who are new to the Mesa<br>
> code base.<br>
><br>
> * If a class initializes all its members in the constructor, we have the<br>
> freedom to allocate it in whatever way is appropriate (stack, rzalloc,<br>
> malloc, new operator, or placement new, to name a few examples). If it<br>
> doesn't, we have no choice but to allocate it using rzalloc.<br>
><br>
> * There's no compile-time mechanism to ensure that all uses of a class<br>
> use rzalloc (nor are there any static analysis tools I'm aware of that<br>
> are capable of ensuring this). It's also a pain to verify using grep.<br>
> So if a class doesn't initialize all its members in the constructor, we<br>
> have to be vigilant in our code review to make sure that all present and<br>
> future users of the class use rzalloc.<br>
<br>
</div>As far as I'm aware, our use of overloaded new / delete with rzalloc<br>
forces us to use new (and therefore rzalloc) for every object or die in<br>
a fire. None of the destructors deallocate any objects owned by them.<br>
Even if we did switch to a different allocation scheme, we would leak<br>
like a screen door on a submarine.<br>
<div><br>
> * By contrast, Coverity *is* able to ensure that class members are<br>
> initialized in the constructor, and I suspect other static analysis<br>
> tools are too.<br>
<br>
</div>Relying on Vinson to run Coverity and submit bugs is not a solid plan.<br>
We have no other plan involving static analysis tools, and we don't have<br>
the staff to filter the flood of false positives that ever static<br>
analysis tool I've ever encountered generates.<br>
<br>
</blockquote></div><br></div><div class="gmail_extra">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: <a href="https://scan.coverity.com/projects/139" target="_blank">https://scan.coverity.com/projects/139</a>, and anyone can sign up to receive e-mail notifications of new defects. I'm signed up to receive the emails too.<br>
</div></div>