ic
/
magnum
mirror of https://github.com/mosra/magnum.git
6
0
Fork 0
Code Issues Projects Releases Wiki Activity
Browse Source

Using std::unique_ptr instead of naked pointers in internal implementation. 

Fixes two accidental memory leaks. Thanks, Clang AddressSanitizer!
pull/77/head
Vladimír Vondruš 12 years ago
parent
9d6c52cd92
commit
eb4eda48f5
6 changed files with 77 additions and 84 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Unified View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 36
      src/Magnum/AbstractFramebuffer.cpp
  2. 58
      src/Magnum/AbstractTexture.cpp
  3. 4
      src/Magnum/DefaultFramebuffer.cpp
  4. 8
      src/Magnum/Framebuffer.cpp
  5. 31
      src/Magnum/Implementation/State.cpp
  6. 24
      src/Magnum/Implementation/State.h

36
src/Magnum/AbstractFramebuffer.cpp
Unescape View File

@ -99,18 +99,18 @@ void AbstractFramebuffer::bind(FramebufferTarget target) {
} }
void AbstractFramebuffer::bindInternal(FramebufferTarget target) { void AbstractFramebuffer::bindInternal(FramebufferTarget target) {
Implementation::FramebufferState* state = Context::current()->state().framebuffer; Implementation::FramebufferState& state = *Context::current()->state().framebuffer;
/* If already bound, done, otherwise update tracked state */ /* If already bound, done, otherwise update tracked state */
if(target == FramebufferTarget::Read) { if(target == FramebufferTarget::Read) {
if(state->readBinding == _id) return; if(state.readBinding == _id) return;
state->readBinding = _id; state.readBinding = _id;
} else if(target == FramebufferTarget::Draw) { } else if(target == FramebufferTarget::Draw) {
if(state->drawBinding == _id) return; if(state.drawBinding == _id) return;
state->drawBinding = _id; state.drawBinding = _id;
} else if(target == FramebufferTarget::ReadDraw) { } else if(target == FramebufferTarget::ReadDraw) {
if(state->readBinding == _id && state->drawBinding == _id) return; if(state.readBinding == _id && state.drawBinding == _id) return;
state->readBinding = state->drawBinding = _id; state.readBinding = state.drawBinding = _id;
} else CORRADE_ASSERT_UNREACHABLE(); } else CORRADE_ASSERT_UNREACHABLE();
/* Binding the framebuffer finally creates it */ /* Binding the framebuffer finally creates it */
@ -119,18 +119,18 @@ void AbstractFramebuffer::bindInternal(FramebufferTarget target) {
} }
FramebufferTarget AbstractFramebuffer::bindInternal() { FramebufferTarget AbstractFramebuffer::bindInternal() {
Implementation::FramebufferState* state = Context::current()->state().framebuffer; Implementation::FramebufferState& state = *Context::current()->state().framebuffer;
/* Return target to which the framebuffer is already bound */ /* Return target to which the framebuffer is already bound */
if(state->readBinding == _id && state->drawBinding == _id) if(state.readBinding == _id && state.drawBinding == _id)
return FramebufferTarget::ReadDraw; return FramebufferTarget::ReadDraw;
if(state->readBinding == _id) if(state.readBinding == _id)
return FramebufferTarget::Read; return FramebufferTarget::Read;
if(state->drawBinding == _id) if(state.drawBinding == _id)
return FramebufferTarget::Draw; return FramebufferTarget::Draw;
/* Or bind it, if not already */ /* Or bind it, if not already */
state->readBinding = _id; state.readBinding = _id;
/* Binding the framebuffer finally creates it */ /* Binding the framebuffer finally creates it */
_created = true; _created = true;
@ -138,8 +138,8 @@ FramebufferTarget AbstractFramebuffer::bindInternal() {
glBindFramebuffer(GLenum(FramebufferTarget::Read), _id); glBindFramebuffer(GLenum(FramebufferTarget::Read), _id);
return FramebufferTarget::Read; return FramebufferTarget::Read;
#else #else
if(state->readTarget == FramebufferTarget::ReadDraw) state->drawBinding = _id; if(state->readTarget == FramebufferTarget::ReadDraw) state.drawBinding = _id;
glBindFramebuffer(GLenum(state->readTarget), _id); glBindFramebuffer(GLenum(state.readTarget), _id);
return state->readTarget; return state->readTarget;
#endif #endif
} }
@ -191,18 +191,18 @@ AbstractFramebuffer& AbstractFramebuffer::setViewport(const Range2Di& rectangle)
} }
void AbstractFramebuffer::setViewportInternal() { void AbstractFramebuffer::setViewportInternal() {
Implementation::FramebufferState* state = Context::current()->state().framebuffer; Implementation::FramebufferState& state = *Context::current()->state().framebuffer;
/* We are using empty viewport to indicate disengaged state */ /* We are using empty viewport to indicate disengaged state */
CORRADE_INTERNAL_ASSERT(_viewport != Range2Di{}); CORRADE_INTERNAL_ASSERT(_viewport != Range2Di{});
CORRADE_INTERNAL_ASSERT(state->drawBinding == _id); CORRADE_INTERNAL_ASSERT(state.drawBinding == _id);
/* Already up-to-date, nothing to do */ /* Already up-to-date, nothing to do */
if(state->viewport == _viewport) if(state.viewport == _viewport)
return; return;
/* Update the state and viewport */ /* Update the state and viewport */
state->viewport = _viewport; state.viewport = _viewport;
glViewport(_viewport.left(), _viewport.bottom(), _viewport.sizeX(), _viewport.sizeY()); glViewport(_viewport.left(), _viewport.bottom(), _viewport.sizeX(), _viewport.sizeY());
} }

58
src/Magnum/AbstractTexture.cpp
Unescape View File

@ -116,25 +116,25 @@ Int AbstractTexture::maxIntegerSamples() {
#endif #endif
void AbstractTexture::unbind(const Int textureUnit) { void AbstractTexture::unbind(const Int textureUnit) {
Implementation::TextureState* const textureState = Context::current()->state().texture; Implementation::TextureState& textureState = *Context::current()->state().texture;
/* If given texture unit is already unbound, nothing to do */ /* If given texture unit is already unbound, nothing to do */
if(textureState->bindings[textureUnit].second == 0) return; if(textureState.bindings[textureUnit].second == 0) return;
/* Unbind the texture, reset state tracker */ /* Unbind the texture, reset state tracker */
Context::current()->state().texture->unbindImplementation(textureUnit); Context::current()->state().texture->unbindImplementation(textureUnit);
textureState->bindings[textureUnit] = {}; textureState.bindings[textureUnit] = {};
} }
void AbstractTexture::unbindImplementationDefault(const GLint textureUnit) { void AbstractTexture::unbindImplementationDefault(const GLint textureUnit) {
Implementation::TextureState* const textureState = Context::current()->state().texture; Implementation::TextureState& textureState = *Context::current()->state().texture;
/* Activate given texture unit if not already active, update state tracker */ /* Activate given texture unit if not already active, update state tracker */
if(textureState->currentTextureUnit != textureUnit) if(textureState.currentTextureUnit != textureUnit)
glActiveTexture(GL_TEXTURE0 + (textureState->currentTextureUnit = textureUnit)); glActiveTexture(GL_TEXTURE0 + (textureState.currentTextureUnit = textureUnit));
CORRADE_INTERNAL_ASSERT(textureState->bindings[textureUnit].first != 0); CORRADE_INTERNAL_ASSERT(textureState.bindings[textureUnit].first != 0);
glBindTexture(textureState->bindings[textureUnit].first, 0); glBindTexture(textureState.bindings[textureUnit].first, 0);
} }
#ifndef MAGNUM_TARGET_GLES #ifndef MAGNUM_TARGET_GLES
@ -144,10 +144,10 @@ void AbstractTexture::unbindImplementationMulti(const GLint textureUnit) {
} }
void AbstractTexture::unbindImplementationDSAEXT(const GLint textureUnit) { void AbstractTexture::unbindImplementationDSAEXT(const GLint textureUnit) {
Implementation::TextureState* const textureState = Context::current()->state().texture; Implementation::TextureState& textureState = *Context::current()->state().texture;
CORRADE_INTERNAL_ASSERT(textureState->bindings[textureUnit].first != 0); CORRADE_INTERNAL_ASSERT(textureState.bindings[textureUnit].first != 0);
glBindMultiTextureEXT(GL_TEXTURE0 + textureUnit, textureState->bindings[textureUnit].first, 0); glBindMultiTextureEXT(GL_TEXTURE0 + textureUnit, textureState.bindings[textureUnit].first, 0);
} }
#endif #endif
@ -170,7 +170,7 @@ void AbstractTexture::bindImplementationFallback(const GLint firstTextureUnit, c
#ifndef MAGNUM_TARGET_GLES #ifndef MAGNUM_TARGET_GLES
/** @todoc const Containers::ArrayReference makes Doxygen grumpy */ /** @todoc const Containers::ArrayReference makes Doxygen grumpy */
void AbstractTexture::bindImplementationMulti(const GLint firstTextureUnit, Containers::ArrayReference<AbstractTexture* const> textures) { void AbstractTexture::bindImplementationMulti(const GLint firstTextureUnit, Containers::ArrayReference<AbstractTexture* const> textures) {
Implementation::TextureState* const textureState = Context::current()->state().texture; Implementation::TextureState& textureState = *Context::current()->state().texture;
/* Create array of IDs and also update bindings in state tracker */ /* Create array of IDs and also update bindings in state tracker */
Containers::Array<GLuint> ids{textures ? textures.size() : 0}; Containers::Array<GLuint> ids{textures ? textures.size() : 0};
@ -183,9 +183,9 @@ void AbstractTexture::bindImplementationMulti(const GLint firstTextureUnit, Cont
ids[i] = id; ids[i] = id;
} }
if(textureState->bindings[firstTextureUnit + i].second != id) { if(textureState.bindings[firstTextureUnit + i].second != id) {
different = true; different = true;
textureState->bindings[firstTextureUnit + i].second = id; textureState.bindings[firstTextureUnit + i].second = id;
} }
} }
@ -246,22 +246,22 @@ AbstractTexture& AbstractTexture::setLabelInternal(const Containers::ArrayRefere
} }
void AbstractTexture::bind(Int textureUnit) { void AbstractTexture::bind(Int textureUnit) {
Implementation::TextureState* const textureState = Context::current()->state().texture; Implementation::TextureState& textureState = *Context::current()->state().texture;
/* If already bound in given texture unit, nothing to do */ /* If already bound in given texture unit, nothing to do */
if(textureState->bindings[textureUnit].second == _id) return; if(textureState.bindings[textureUnit].second == _id) return;
/* Update state tracker, bind the texture to the unit */ /* Update state tracker, bind the texture to the unit */
textureState->bindings[textureUnit] = {_target, _id}; textureState.bindings[textureUnit] = {_target, _id};
(this->*Context::current()->state().texture->bindImplementation)(textureUnit); (this->*textureState.bindImplementation)(textureUnit);
} }
void AbstractTexture::bindImplementationDefault(GLint textureUnit) { void AbstractTexture::bindImplementationDefault(GLint textureUnit) {
Implementation::TextureState* const textureState = Context::current()->state().texture; Implementation::TextureState& textureState = *Context::current()->state().texture;
/* Activate given texture unit if not already active, update state tracker */ /* Activate given texture unit if not already active, update state tracker */
if(textureState->currentTextureUnit != textureUnit) if(textureState.currentTextureUnit != textureUnit)
glActiveTexture(GL_TEXTURE0 + (textureState->currentTextureUnit = textureUnit)); glActiveTexture(GL_TEXTURE0 + (textureState.currentTextureUnit = textureUnit));
/* Binding the texture finally creates it */ /* Binding the texture finally creates it */
_created = true; _created = true;
@ -412,21 +412,21 @@ void AbstractTexture::bindInternal() {
functions need to have the texture bound in *currently active* unit, functions need to have the texture bound in *currently active* unit,
so we would need to call glActiveTexture() afterwards anyway. */ so we would need to call glActiveTexture() afterwards anyway. */
Implementation::TextureState* const textureState = Context::current()->state().texture; Implementation::TextureState& textureState = *Context::current()->state().texture;
/* If the texture is already bound in current unit, nothing to do */ /* If the texture is already bound in current unit, nothing to do */
if(textureState->bindings[textureState->currentTextureUnit].second == _id) if(textureState.bindings[textureState.currentTextureUnit].second == _id)
return; return;
/* Set internal unit as active if not already, update state tracker */ /* Set internal unit as active if not already, update state tracker */
CORRADE_INTERNAL_ASSERT(textureState->maxTextureUnits > 1); CORRADE_INTERNAL_ASSERT(textureState.maxTextureUnits > 1);
const GLint internalTextureUnit = textureState->maxTextureUnits-1; const GLint internalTextureUnit = textureState.maxTextureUnits-1;
if(textureState->currentTextureUnit != internalTextureUnit) if(textureState.currentTextureUnit != internalTextureUnit)
glActiveTexture(GL_TEXTURE0 + (textureState->currentTextureUnit = internalTextureUnit)); glActiveTexture(GL_TEXTURE0 + (textureState.currentTextureUnit = internalTextureUnit));
/* Bind the texture to internal unit if not already, update state tracker */ /* Bind the texture to internal unit if not already, update state tracker */
if(textureState->bindings[internalTextureUnit].second == _id) return; if(textureState.bindings[internalTextureUnit].second == _id) return;
textureState->bindings[internalTextureUnit] = {_target, _id}; textureState.bindings[internalTextureUnit] = {_target, _id};
/* Binding the texture finally creates it */ /* Binding the texture finally creates it */
_created = true; _created = true;

4
src/Magnum/DefaultFramebuffer.cpp
Unescape View File

@ -95,12 +95,12 @@ void DefaultFramebuffer::invalidate(std::initializer_list<InvalidationAttachment
#endif #endif
void DefaultFramebuffer::initializeContextBasedFunctionality(Context& context) { void DefaultFramebuffer::initializeContextBasedFunctionality(Context& context) {
Implementation::FramebufferState* state = context.state().framebuffer; Implementation::FramebufferState& state = *context.state().framebuffer;
/* Initial framebuffer size */ /* Initial framebuffer size */
GLint viewport[4]; GLint viewport[4];
glGetIntegerv(GL_VIEWPORT, viewport); glGetIntegerv(GL_VIEWPORT, viewport);
defaultFramebuffer._viewport = state->viewport = Range2Di::fromSize({viewport[0], viewport[1]}, {viewport[2], viewport[3]}); defaultFramebuffer._viewport = state.viewport = Range2Di::fromSize({viewport[0], viewport[1]}, {viewport[2], viewport[3]});
/* Fake initial glViewport() call for ApiTrace */ /* Fake initial glViewport() call for ApiTrace */
#ifndef MAGNUM_TARGET_GLES #ifndef MAGNUM_TARGET_GLES

8
src/Magnum/Framebuffer.cpp
Unescape View File

@ -102,12 +102,12 @@ Framebuffer::~Framebuffer() {
if(!_id) return; if(!_id) return;
/* If bound, remove itself from state */ /* If bound, remove itself from state */
Implementation::FramebufferState* state = Context::current()->state().framebuffer; Implementation::FramebufferState& state = *Context::current()->state().framebuffer;
if(state->readBinding == _id) state->readBinding = 0; if(state.readBinding == _id) state.readBinding = 0;
/* For draw binding reset also viewport */ /* For draw binding reset also viewport */
if(state->drawBinding == _id) { if(state.drawBinding == _id) {
state->drawBinding = 0; state.drawBinding = 0;
/** /**
* @todo Less ugly solution (need to call setViewportInternal() to * @todo Less ugly solution (need to call setViewportInternal() to

31
src/Magnum/Implementation/State.cpp
Unescape View File

@ -55,17 +55,17 @@ State::State(Context& context) {
extensions.reserve(8); extensions.reserve(8);
#endif #endif
buffer = new BufferState(context, extensions); buffer.reset(new BufferState{context, extensions});
debug = new DebugState(context, extensions); debug.reset(new DebugState{context, extensions});
framebuffer = new FramebufferState(context, extensions); framebuffer.reset(new FramebufferState{context, extensions});
mesh = new MeshState(context, extensions); mesh.reset(new MeshState{context, extensions});
query = new QueryState(context, extensions); query.reset(new QueryState{context, extensions});
renderer = new RendererState(context, extensions); renderer.reset(new RendererState{context, extensions});
shader = new ShaderState; shader.reset(new ShaderState);
shaderProgram = new ShaderProgramState(context, extensions); shaderProgram.reset(new ShaderProgramState{context, extensions});
texture = new TextureState(context, extensions); texture.reset(new TextureState{context, extensions});
#ifndef MAGNUM_TARGET_GLES2 #ifndef MAGNUM_TARGET_GLES2
transformFeedback = new TransformFeedbackState(context, extensions); transformFeedback.reset(new TransformFeedbackState{context, extensions});
#endif #endif
/* Sort the features and remove duplicates */ /* Sort the features and remove duplicates */
@ -76,15 +76,6 @@ State::State(Context& context) {
for(const auto& ext: extensions) Debug() << " " << ext; for(const auto& ext: extensions) Debug() << " " << ext;
} }
State::~State() { State::~State() = default;
delete texture;
delete shaderProgram;
delete shader;
delete renderer;
delete mesh;
delete framebuffer;
delete debug;
delete buffer;
}
}} }}

24
src/Magnum/Implementation/State.h
Unescape View File

@ -25,6 +25,8 @@
DEALINGS IN THE SOFTWARE. DEALINGS IN THE SOFTWARE.
*/ */
#include <memory>
#include "Magnum/Magnum.h" #include "Magnum/Magnum.h"
#include "Magnum/OpenGL.h" #include "Magnum/OpenGL.h"
@ -45,23 +47,23 @@ struct TransformFeedbackState;
struct State { struct State {
/* Initializes context-based functionality */ /* Initializes context-based functionality */
State(Context& context); explicit State(Context& context);
~State(); ~State();
enum: GLuint { DisengagedBinding = ~0u }; enum: GLuint { DisengagedBinding = ~0u };
BufferState* buffer; std::unique_ptr<BufferState> buffer;
DebugState* debug; std::unique_ptr<DebugState> debug;
FramebufferState* framebuffer; std::unique_ptr<FramebufferState> framebuffer;
MeshState* mesh; std::unique_ptr<MeshState> mesh;
QueryState* query; std::unique_ptr<QueryState> query;
RendererState* renderer; std::unique_ptr<RendererState> renderer;
ShaderState* shader; std::unique_ptr<ShaderState> shader;
ShaderProgramState* shaderProgram; std::unique_ptr<ShaderProgramState> shaderProgram;
TextureState* texture; std::unique_ptr<TextureState> texture;
#ifndef MAGNUM_TARGET_GLES2 #ifndef MAGNUM_TARGET_GLES2
TransformFeedbackState* transformFeedback; std::unique_ptr<TransformFeedbackState> transformFeedback;
#endif #endif
}; };

Write Preview
Loading…
Cancel
Save