[opensource-dev] Review Request: patch potential memory leak in llgl.h
Boroondas Gupte
sllists at boroon.dasgupta.ch
Fri Sep 21 14:10:20 PDT 2012
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/603/#review1270
-----------------------------------------------------------
indra/llrender/llgl.h
<http://codereview.secondlife.com/r/603/#comment1160>
Why did you place two exclamation marks in the comment?
Also, I think "destructor" was actually the correct term while "destroyer" isn't.
But as Carlo already mentioned: This explanation would better be placed in the commit message than the code. While even experienced programmers might forget to write a virtual destructor where needed, we can probably expect them to know why it's got to be there when they see one.
If you feel there needs to be an in-code comment anyway, how about something short like:
// needed because class has virtual methods
- Boroondas Gupte
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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20120921/cc960693/attachment.htm
More information about the opensource-dev
mailing list