On 18 September 2013 13:15, Ian Romanick <i...@freedesktop.org> wrote:
> On 09/18/2013 01:35 PM, Paul Berry wrote: > > On 18 September 2013 07:43, Ian Romanick <i...@freedesktop.org > > <mailto:i...@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.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
