- Boroondas
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/603/#review1268 ----------------------------------------------------------- On Sept. 20, 2012, 6:15 p.m., Gistya Eusebio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/603/ > ----------------------------------------------------------- > > (Updated Sept. 20, 2012, 6:15 p.m.) > > > Review request for Viewer. > > > Description > ------- > > In llvertexbuffer.cpp we call: delete mFence; > > mFence is an instance of class LLGLSyncFence which is a sub-class of > LLGLFence, which is defined in llgl.h. > > However, class LLGLFence should have a virtual destructor because it's the > base class. This patch fixes that potential memory leak, adding a virtual > destructor to class LLGLFence. The virtual destructor ensures that the > destructor for the derived class also gets called when we call "delete > mfence;". > > To quote Björn Pollex, "If you have a class that is supposed to be usable > polymorphically, it should also be deletable polymorphically." > > (Unless I'm missing something...!) > > NOTE: I notice that related code is commented in methods "void > LLVertexBuffer::placeFence() const" and "void LLVertexBuffer::waitFence() > const" -- maybe we commented it out because this memory leak was unresolved? > Perhaps it can be uncommented now? I haven't tried yet. There was no note as > to why the code is commented out. > > > Diffs > ----- > > indra/llrender/llgl.h UNKNOWN > > Diff: http://codereview.secondlife.com/r/603/diff/ > > > Testing > ------- > > I did compile this with OS X 10.8 as a build target successfully. I made > other changes too, so while my FPS seems improved, it could be from any > number of issues. I did notice that any llCharacters that are moving around > don't get rendered properly by my build, but I don't know if it's because of > this code revision or something else. I need to do further testing on that. > > > Thanks, > > Gistya Eusebio > >
_______________________________________________ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource-Dev Please read the policies before posting to keep unmoderated posting privileges
