JavaScript is turned off

Review Board requires JavaScript in order to function. Please turn it on in your browser preferences.

Firefox users: if you prefer to turn on JavaScript only for specific sites, we recommend the NoScript extension.


Submitter:	Gistya Eusebio	Reviewers
Branch:		Groups:	viewer
Bugs:		People:
Change Number:	None	Repository:	viewer-development

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.

Testing Done:

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.

Expand All

There is semi-colon where it doesn't belong. Otherwise ok (Singularity has this patch since a long time).

Gistya Eusebio Sept. 20, 2012, 12:08 a.m. (Sept. 20, 2012, 12:08 a.m.)
```
Oh really, where? It compiled and ran fine on my machine.
```

What Carlo said:

indra/llrender/llgl.h (Diff revision 1)

The difference in indentation can easily be seen here (at least in my browser).

indra/llrender/llgl.h (Diff revision 1)

After a function body, no semicolon is needed, whether that body is empty or not. (The semicolons on the preexisting lines above are different, as these functions don't have a body.)

Gistya Eusebio Sept. 20, 2012, 6:26 p.m. (Sept. 20, 2012, 6:26 p.m.)

You're right, it's unneeded. I think it was left over from when at first I had used the pure virtual destructor:
virtual ~LLGLFence() = 0;
but there are already some pure virtual functions in class LLGLFence forcing themselves to be overridden and the proposed solution just seemed more elegant, if less pure.

Diff
- added Diff r2

Description:

Fixed the harmless (but unneeded) semicolon and indentation that used spaces.

indra/llrender/llgl.h (Diff revisions 1 - 2)

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

You have a pending review.

Other reviews

Your comment

Review Board 1.6.11

patch potential memory leak in llgl.h