[opensource-dev] Review Request: patch potential memory leak in llgl.h

[Carlo Wood]

On Thu, 20 Sep 2012 07:08:11 -0000
"Gistya Eusebio" <gistya at gmail.com> wrote:

> 
> 
> > On Sept. 19, 2012, 2:12 p.m., Aleric Inglewood wrote:
> > > There is semi-colon where it doesn't belong. Otherwise ok
> > > (Singularity has this patch since a long time).
> 
> Oh really, where? It compiled and ran fine on my machine.

Perhaps it's the ONLY semi-colon you added?
Semi-colons have the tendency to compile if you add
one at random; still doesn't belong there though.
With regard to white-space, you also should start
the line with a TAB instead of spaces to match the
already existing lines, and the added comment is something
that should be in the commit message, not in the code.

> 
> 
> - Gistya
> 
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/603/#review1265
> -----------------------------------------------------------
> 
> 
> On Sept. 19, 2012, 8:26 a.m., Gistya Eusebio wrote:
> > 
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > http://codereview.secondlife.com/r/603/
> > -----------------------------------------------------------
> > 
> > (Updated Sept. 19, 2012, 8:26 a.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
> > 
> >
> 



-- 
Carlo Wood <carlo at alinoe.com>