From eb4eda48f5dd2b3d1688b1bc7226b8bd5a11ac20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sat, 1 Nov 2014 22:34:26 +0100 Subject: [PATCH] Using std::unique_ptr instead of naked pointers in internal implementation. Fixes two accidental memory leaks. Thanks, Clang AddressSanitizer! --- src/Magnum/AbstractFramebuffer.cpp | 36 +++++++++--------- src/Magnum/AbstractTexture.cpp | 58 ++++++++++++++--------------- src/Magnum/DefaultFramebuffer.cpp | 4 +- src/Magnum/Framebuffer.cpp | 8 ++-- src/Magnum/Implementation/State.cpp | 31 ++++++--------- src/Magnum/Implementation/State.h | 24 ++++++------ 6 files changed, 77 insertions(+), 84 deletions(-) diff --git a/src/Magnum/AbstractFramebuffer.cpp b/src/Magnum/AbstractFramebuffer.cpp index 80477b214..bd607956f 100644 --- a/src/Magnum/AbstractFramebuffer.cpp +++ b/src/Magnum/AbstractFramebuffer.cpp @@ -99,18 +99,18 @@ void AbstractFramebuffer::bind(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(target == FramebufferTarget::Read) { - if(state->readBinding == _id) return; - state->readBinding = _id; + if(state.readBinding == _id) return; + state.readBinding = _id; } else if(target == FramebufferTarget::Draw) { - if(state->drawBinding == _id) return; - state->drawBinding = _id; + if(state.drawBinding == _id) return; + state.drawBinding = _id; } else if(target == FramebufferTarget::ReadDraw) { - if(state->readBinding == _id && state->drawBinding == _id) return; - state->readBinding = state->drawBinding = _id; + if(state.readBinding == _id && state.drawBinding == _id) return; + state.readBinding = state.drawBinding = _id; } else CORRADE_ASSERT_UNREACHABLE(); /* Binding the framebuffer finally creates it */ @@ -119,18 +119,18 @@ void AbstractFramebuffer::bindInternal(FramebufferTarget target) { } 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 */ - if(state->readBinding == _id && state->drawBinding == _id) + if(state.readBinding == _id && state.drawBinding == _id) return FramebufferTarget::ReadDraw; - if(state->readBinding == _id) + if(state.readBinding == _id) return FramebufferTarget::Read; - if(state->drawBinding == _id) + if(state.drawBinding == _id) return FramebufferTarget::Draw; /* Or bind it, if not already */ - state->readBinding = _id; + state.readBinding = _id; /* Binding the framebuffer finally creates it */ _created = true; @@ -138,8 +138,8 @@ FramebufferTarget AbstractFramebuffer::bindInternal() { glBindFramebuffer(GLenum(FramebufferTarget::Read), _id); return FramebufferTarget::Read; #else - if(state->readTarget == FramebufferTarget::ReadDraw) state->drawBinding = _id; - glBindFramebuffer(GLenum(state->readTarget), _id); + if(state->readTarget == FramebufferTarget::ReadDraw) state.drawBinding = _id; + glBindFramebuffer(GLenum(state.readTarget), _id); return state->readTarget; #endif } @@ -191,18 +191,18 @@ AbstractFramebuffer& AbstractFramebuffer::setViewport(const Range2Di& rectangle) } 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 */ CORRADE_INTERNAL_ASSERT(_viewport != Range2Di{}); - CORRADE_INTERNAL_ASSERT(state->drawBinding == _id); + CORRADE_INTERNAL_ASSERT(state.drawBinding == _id); /* Already up-to-date, nothing to do */ - if(state->viewport == _viewport) + if(state.viewport == _viewport) return; /* Update the state and viewport */ - state->viewport = _viewport; + state.viewport = _viewport; glViewport(_viewport.left(), _viewport.bottom(), _viewport.sizeX(), _viewport.sizeY()); } diff --git a/src/Magnum/AbstractTexture.cpp b/src/Magnum/AbstractTexture.cpp index 35f94797a..48eea2eb3 100644 --- a/src/Magnum/AbstractTexture.cpp +++ b/src/Magnum/AbstractTexture.cpp @@ -116,25 +116,25 @@ Int AbstractTexture::maxIntegerSamples() { #endif 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(textureState->bindings[textureUnit].second == 0) return; + if(textureState.bindings[textureUnit].second == 0) return; /* Unbind the texture, reset state tracker */ Context::current()->state().texture->unbindImplementation(textureUnit); - textureState->bindings[textureUnit] = {}; + textureState.bindings[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 */ - if(textureState->currentTextureUnit != textureUnit) - glActiveTexture(GL_TEXTURE0 + (textureState->currentTextureUnit = textureUnit)); + if(textureState.currentTextureUnit != textureUnit) + glActiveTexture(GL_TEXTURE0 + (textureState.currentTextureUnit = textureUnit)); - CORRADE_INTERNAL_ASSERT(textureState->bindings[textureUnit].first != 0); - glBindTexture(textureState->bindings[textureUnit].first, 0); + CORRADE_INTERNAL_ASSERT(textureState.bindings[textureUnit].first != 0); + glBindTexture(textureState.bindings[textureUnit].first, 0); } #ifndef MAGNUM_TARGET_GLES @@ -144,10 +144,10 @@ void AbstractTexture::unbindImplementationMulti(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); - glBindMultiTextureEXT(GL_TEXTURE0 + textureUnit, textureState->bindings[textureUnit].first, 0); + CORRADE_INTERNAL_ASSERT(textureState.bindings[textureUnit].first != 0); + glBindMultiTextureEXT(GL_TEXTURE0 + textureUnit, textureState.bindings[textureUnit].first, 0); } #endif @@ -170,7 +170,7 @@ void AbstractTexture::bindImplementationFallback(const GLint firstTextureUnit, c #ifndef MAGNUM_TARGET_GLES /** @todoc const Containers::ArrayReference makes Doxygen grumpy */ void AbstractTexture::bindImplementationMulti(const GLint firstTextureUnit, Containers::ArrayReference 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 */ Containers::Array ids{textures ? textures.size() : 0}; @@ -183,9 +183,9 @@ void AbstractTexture::bindImplementationMulti(const GLint firstTextureUnit, Cont ids[i] = id; } - if(textureState->bindings[firstTextureUnit + i].second != id) { + if(textureState.bindings[firstTextureUnit + i].second != id) { 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) { - 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(textureState->bindings[textureUnit].second == _id) return; + if(textureState.bindings[textureUnit].second == _id) return; /* Update state tracker, bind the texture to the unit */ - textureState->bindings[textureUnit] = {_target, _id}; - (this->*Context::current()->state().texture->bindImplementation)(textureUnit); + textureState.bindings[textureUnit] = {_target, _id}; + (this->*textureState.bindImplementation)(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 */ - if(textureState->currentTextureUnit != textureUnit) - glActiveTexture(GL_TEXTURE0 + (textureState->currentTextureUnit = textureUnit)); + if(textureState.currentTextureUnit != textureUnit) + glActiveTexture(GL_TEXTURE0 + (textureState.currentTextureUnit = textureUnit)); /* Binding the texture finally creates it */ _created = true; @@ -412,21 +412,21 @@ void AbstractTexture::bindInternal() { functions need to have the texture bound in *currently active* unit, 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(textureState->bindings[textureState->currentTextureUnit].second == _id) + if(textureState.bindings[textureState.currentTextureUnit].second == _id) return; /* Set internal unit as active if not already, update state tracker */ - CORRADE_INTERNAL_ASSERT(textureState->maxTextureUnits > 1); - const GLint internalTextureUnit = textureState->maxTextureUnits-1; - if(textureState->currentTextureUnit != internalTextureUnit) - glActiveTexture(GL_TEXTURE0 + (textureState->currentTextureUnit = internalTextureUnit)); + CORRADE_INTERNAL_ASSERT(textureState.maxTextureUnits > 1); + const GLint internalTextureUnit = textureState.maxTextureUnits-1; + if(textureState.currentTextureUnit != internalTextureUnit) + glActiveTexture(GL_TEXTURE0 + (textureState.currentTextureUnit = internalTextureUnit)); /* Bind the texture to internal unit if not already, update state tracker */ - if(textureState->bindings[internalTextureUnit].second == _id) return; - textureState->bindings[internalTextureUnit] = {_target, _id}; + if(textureState.bindings[internalTextureUnit].second == _id) return; + textureState.bindings[internalTextureUnit] = {_target, _id}; /* Binding the texture finally creates it */ _created = true; diff --git a/src/Magnum/DefaultFramebuffer.cpp b/src/Magnum/DefaultFramebuffer.cpp index 034f37cc7..b5d3e9654 100644 --- a/src/Magnum/DefaultFramebuffer.cpp +++ b/src/Magnum/DefaultFramebuffer.cpp @@ -95,12 +95,12 @@ void DefaultFramebuffer::invalidate(std::initializer_listviewport = 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 */ #ifndef MAGNUM_TARGET_GLES diff --git a/src/Magnum/Framebuffer.cpp b/src/Magnum/Framebuffer.cpp index b2884c749..8dfc13487 100644 --- a/src/Magnum/Framebuffer.cpp +++ b/src/Magnum/Framebuffer.cpp @@ -102,12 +102,12 @@ Framebuffer::~Framebuffer() { if(!_id) return; /* If bound, remove itself from state */ - Implementation::FramebufferState* state = Context::current()->state().framebuffer; - if(state->readBinding == _id) state->readBinding = 0; + Implementation::FramebufferState& state = *Context::current()->state().framebuffer; + if(state.readBinding == _id) state.readBinding = 0; /* For draw binding reset also viewport */ - if(state->drawBinding == _id) { - state->drawBinding = 0; + if(state.drawBinding == _id) { + state.drawBinding = 0; /** * @todo Less ugly solution (need to call setViewportInternal() to diff --git a/src/Magnum/Implementation/State.cpp b/src/Magnum/Implementation/State.cpp index 935f5cd22..cda0b4554 100644 --- a/src/Magnum/Implementation/State.cpp +++ b/src/Magnum/Implementation/State.cpp @@ -55,17 +55,17 @@ State::State(Context& context) { extensions.reserve(8); #endif - buffer = new BufferState(context, extensions); - debug = new DebugState(context, extensions); - framebuffer = new FramebufferState(context, extensions); - mesh = new MeshState(context, extensions); - query = new QueryState(context, extensions); - renderer = new RendererState(context, extensions); - shader = new ShaderState; - shaderProgram = new ShaderProgramState(context, extensions); - texture = new TextureState(context, extensions); + buffer.reset(new BufferState{context, extensions}); + debug.reset(new DebugState{context, extensions}); + framebuffer.reset(new FramebufferState{context, extensions}); + mesh.reset(new MeshState{context, extensions}); + query.reset(new QueryState{context, extensions}); + renderer.reset(new RendererState{context, extensions}); + shader.reset(new ShaderState); + shaderProgram.reset(new ShaderProgramState{context, extensions}); + texture.reset(new TextureState{context, extensions}); #ifndef MAGNUM_TARGET_GLES2 - transformFeedback = new TransformFeedbackState(context, extensions); + transformFeedback.reset(new TransformFeedbackState{context, extensions}); #endif /* Sort the features and remove duplicates */ @@ -76,15 +76,6 @@ State::State(Context& context) { for(const auto& ext: extensions) Debug() << " " << ext; } -State::~State() { - delete texture; - delete shaderProgram; - delete shader; - delete renderer; - delete mesh; - delete framebuffer; - delete debug; - delete buffer; -} +State::~State() = default; }} diff --git a/src/Magnum/Implementation/State.h b/src/Magnum/Implementation/State.h index d1e7e1dcf..758b61997 100644 --- a/src/Magnum/Implementation/State.h +++ b/src/Magnum/Implementation/State.h @@ -25,6 +25,8 @@ DEALINGS IN THE SOFTWARE. */ +#include + #include "Magnum/Magnum.h" #include "Magnum/OpenGL.h" @@ -45,23 +47,23 @@ struct TransformFeedbackState; struct State { /* Initializes context-based functionality */ - State(Context& context); + explicit State(Context& context); ~State(); enum: GLuint { DisengagedBinding = ~0u }; - BufferState* buffer; - DebugState* debug; - FramebufferState* framebuffer; - MeshState* mesh; - QueryState* query; - RendererState* renderer; - ShaderState* shader; - ShaderProgramState* shaderProgram; - TextureState* texture; + std::unique_ptr buffer; + std::unique_ptr debug; + std::unique_ptr framebuffer; + std::unique_ptr mesh; + std::unique_ptr query; + std::unique_ptr renderer; + std::unique_ptr shader; + std::unique_ptr shaderProgram; + std::unique_ptr texture; #ifndef MAGNUM_TARGET_GLES2 - TransformFeedbackState* transformFeedback; + std::unique_ptr transformFeedback; #endif };