From 5f51652aff5c31577b9816b41bbc92414d86b493 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sat, 28 Dec 2013 18:58:20 +0100 Subject: [PATCH] Updated, documented and tested Shader and ShaderProgram movement. Added Shader::type() and Shader::sources() accessors for testing purposes, it involved renaming private members (no functional change). --- src/Shader.cpp | 63 ++++++++----------- src/Shader.h | 36 ++++++++--- src/Test/AbstractShaderProgramGLTest.cpp | 62 +++++++++++++++++-- src/Test/ShaderGLTest.cpp | 78 +++++++++++++++++++++++- 4 files changed, 187 insertions(+), 52 deletions(-) diff --git a/src/Shader.cpp b/src/Shader.cpp index a81b6e997..b6b307893 100644 --- a/src/Shader.cpp +++ b/src/Shader.cpp @@ -540,19 +540,19 @@ Shader::Shader(const Version version, const Type type): _type(type), _id(0) { switch(version) { #ifndef MAGNUM_TARGET_GLES - case Version::GL210: sources.push_back("#version 120\n"); return; - case Version::GL300: sources.push_back("#version 130\n"); return; - case Version::GL310: sources.push_back("#version 140\n"); return; - case Version::GL320: sources.push_back("#version 150\n"); return; - case Version::GL330: sources.push_back("#version 330\n"); return; - case Version::GL400: sources.push_back("#version 400\n"); return; - case Version::GL410: sources.push_back("#version 410\n"); return; - case Version::GL420: sources.push_back("#version 420\n"); return; - case Version::GL430: sources.push_back("#version 430\n"); return; - case Version::GL440: sources.push_back("#version 440\n"); return; + case Version::GL210: _sources.push_back("#version 120\n"); return; + case Version::GL300: _sources.push_back("#version 130\n"); return; + case Version::GL310: _sources.push_back("#version 140\n"); return; + case Version::GL320: _sources.push_back("#version 150\n"); return; + case Version::GL330: _sources.push_back("#version 330\n"); return; + case Version::GL400: _sources.push_back("#version 400\n"); return; + case Version::GL410: _sources.push_back("#version 410\n"); return; + case Version::GL420: _sources.push_back("#version 420\n"); return; + case Version::GL430: _sources.push_back("#version 430\n"); return; + case Version::GL440: _sources.push_back("#version 440\n"); return; #else - case Version::GLES200: sources.push_back("#version 100\n"); return; - case Version::GLES300: sources.push_back("#version 300\n"); return; + case Version::GLES200: _sources.push_back("#version 100\n"); return; + case Version::GLES300: _sources.push_back("#version 300\n"); return; #endif case Version::None: @@ -562,24 +562,11 @@ Shader::Shader(const Version version, const Type type): _type(type), _id(0) { CORRADE_ASSERT_UNREACHABLE(); } -Shader::Shader(Shader&& other): _type(other._type), _id(other._id), sources(std::move(other.sources)) { - other._id = 0; -} - Shader::~Shader() { - if(_id) glDeleteShader(_id); -} + /* Moved out, nothing to do */ + if(!_id) return; -Shader& Shader::operator=(Shader&& other) { glDeleteShader(_id); - - _type = other._type; - sources = std::move(other.sources); - _id = other._id; - - other._id = 0; - - return *this; } std::string Shader::label() const { @@ -599,6 +586,8 @@ Shader& Shader::setLabel(const std::string& label) { return *this; } +std::vector Shader::sources() const { return _sources; } + Shader& Shader::addSource(std::string source) { if(!source.empty()) { #if defined(CORRADE_TARGET_NACL_NEWLIB) || defined(__MINGW32__) @@ -608,14 +597,14 @@ Shader& Shader::addSource(std::string source) { /* Fix line numbers, so line 41 of third added file is marked as 3(41). Source 0 is the #version string added in constructor. */ - sources.push_back("#line 1 " + + _sources.push_back("#line 1 " + #if !defined(CORRADE_TARGET_NACL_NEWLIB) && !defined(__MINGW32__) - std::to_string((sources.size()+1)/2) + + std::to_string((_sources.size()+1)/2) + #else converter.str() + #endif '\n'); - sources.push_back(std::move(source)); + _sources.push_back(std::move(source)); } return *this; @@ -630,20 +619,20 @@ Shader& Shader::addFile(const std::string& filename) { } bool Shader::compile() { - CORRADE_ASSERT(sources.size() > 1, "Shader::compile(): no files added", false); + CORRADE_ASSERT(_sources.size() > 1, "Shader::compile(): no files added", false); /* Array of source string pointers and their lengths */ /** @todo Use `Containers::ArrayTuple` to avoid one allocation if it ever gets to be implemented (we need properly aligned memory too) */ - Containers::Array pointers(sources.size()); - Containers::Array sizes(sources.size()); - for(std::size_t i = 0; i != sources.size(); ++i) { - pointers[i] = static_cast(sources[i].data()); - sizes[i] = sources[i].size(); + Containers::Array pointers(_sources.size()); + Containers::Array sizes(_sources.size()); + for(std::size_t i = 0; i != _sources.size(); ++i) { + pointers[i] = static_cast(_sources[i].data()); + sizes[i] = _sources[i].size(); } /* Create shader and set its source */ - glShaderSource(_id, sources.size(), pointers, sizes); + glShaderSource(_id, _sources.size(), pointers, sizes); /* Compile shader */ glCompileShader(_id); diff --git a/src/Shader.h b/src/Shader.h index 826244d9a..6764fde00 100644 --- a/src/Shader.h +++ b/src/Shader.h @@ -44,9 +44,6 @@ namespace Magnum { See AbstractShaderProgram for more information. */ class MAGNUM_EXPORT Shader: public AbstractObject { - Shader(const Shader&) = delete; - Shader& operator=(const Shader&) = delete; - public: /** * @brief %Shader type @@ -439,6 +436,12 @@ class MAGNUM_EXPORT Shader: public AbstractObject { */ explicit Shader(Version version, Type type); + /** @brief Copying is not allowed */ + Shader(const Shader&) = delete; + + /** @brief Move constructor */ + Shader(Shader&& other) noexcept; + /** * @brief Destructor * @@ -447,11 +450,11 @@ class MAGNUM_EXPORT Shader: public AbstractObject { */ ~Shader(); - /** @brief Move constructor */ - Shader(Shader&& other); + /** @brief Copying is not allowed */ + Shader& operator=(const Shader&) = delete; - /** @brief Move assignment operator */ - Shader& operator=(Shader&& other); + /** @brief Move assignment */ + Shader& operator=(Shader&& other) noexcept; /** @brief OpenGL shader ID */ GLuint id() const { return _id; } @@ -482,6 +485,12 @@ class MAGNUM_EXPORT Shader: public AbstractObject { */ Shader& setLabel(const std::string& label); + /** @brief Shader type */ + Type type() const { return _type; } + + /** @brief Shader sources */ + std::vector sources() const; + /** * @brief Add shader source * @param source String with shader source @@ -519,12 +528,23 @@ class MAGNUM_EXPORT Shader: public AbstractObject { Type _type; GLuint _id; - std::vector sources; + std::vector _sources; }; /** @debugoperator{Magnum::Shader} */ Debug MAGNUM_EXPORT operator<<(Debug debug, Shader::Type value); +inline Shader::Shader(Shader&& other) noexcept: _type(other._type), _id(other._id), _sources(std::move(other._sources)) { + other._id = 0; +} + +inline Shader& Shader::operator=(Shader&& other) noexcept { + std::swap(_type, other._type); + std::swap(_id, other._id); + std::swap(_sources, other._sources); + return *this; +} + } #endif diff --git a/src/Test/AbstractShaderProgramGLTest.cpp b/src/Test/AbstractShaderProgramGLTest.cpp index 3bb3a7a6b..0881927c5 100644 --- a/src/Test/AbstractShaderProgramGLTest.cpp +++ b/src/Test/AbstractShaderProgramGLTest.cpp @@ -33,11 +33,66 @@ class AbstractShaderProgramGLTest: public AbstractOpenGLTester { public: explicit AbstractShaderProgramGLTest(); + void construct(); + void constructCopy(); + void constructMove(); + void label(); }; AbstractShaderProgramGLTest::AbstractShaderProgramGLTest() { - addTests({&AbstractShaderProgramGLTest::label}); + addTests({&AbstractShaderProgramGLTest::construct, + &AbstractShaderProgramGLTest::constructCopy, + &AbstractShaderProgramGLTest::constructMove, + + &AbstractShaderProgramGLTest::label}); +} + +namespace { + +class MyShader: public AbstractShaderProgram { + public: + explicit MyShader() {} +}; + +} + +void AbstractShaderProgramGLTest::construct() { + { + const MyShader shader; + + MAGNUM_VERIFY_NO_ERROR(); + CORRADE_VERIFY(shader.id() > 0); + } + + MAGNUM_VERIFY_NO_ERROR(); +} + +void AbstractShaderProgramGLTest::constructCopy() { + CORRADE_VERIFY(!(std::is_constructible{})); + CORRADE_VERIFY(!(std::is_assignable{})); +} + +void AbstractShaderProgramGLTest::constructMove() { + MyShader a; + const Int id = a.id(); + + MAGNUM_VERIFY_NO_ERROR(); + CORRADE_VERIFY(id > 0); + + MyShader b(std::move(a)); + + CORRADE_COMPARE(a.id(), 0); + CORRADE_COMPARE(b.id(), id); + + MyShader c; + const Int cId = c.id(); + c = std::move(b); + + MAGNUM_VERIFY_NO_ERROR(); + CORRADE_VERIFY(cId > 0); + CORRADE_COMPARE(b.id(), cId); + CORRADE_COMPARE(c.id(), id); } void AbstractShaderProgramGLTest::label() { @@ -46,11 +101,6 @@ void AbstractShaderProgramGLTest::label() { !Context::current()->isExtensionSupported()) CORRADE_SKIP("Required extension is not available"); - class MyShader: public AbstractShaderProgram { - public: - explicit MyShader() {} - }; - MyShader shader; CORRADE_COMPARE(shader.label(), ""); diff --git a/src/Test/ShaderGLTest.cpp b/src/Test/ShaderGLTest.cpp index 43d1d131e..8e5846b8a 100644 --- a/src/Test/ShaderGLTest.cpp +++ b/src/Test/ShaderGLTest.cpp @@ -33,11 +33,87 @@ class ShaderGLTest: public AbstractOpenGLTester { public: explicit ShaderGLTest(); + void construct(); + void constructCopy(); + void constructMove(); + void label(); }; ShaderGLTest::ShaderGLTest() { - addTests({&ShaderGLTest::label}); + addTests({&ShaderGLTest::construct, + &ShaderGLTest::constructCopy, + &ShaderGLTest::constructMove, + + &ShaderGLTest::label}); +} + +void ShaderGLTest::construct() { + { + #ifndef MAGNUM_TARGET_GLES + const Shader shader(Version::GL300, Shader::Type::Fragment); + #else + const Shader shader(Version::GLES300, Shader::Type::Fragment); + #endif + + MAGNUM_VERIFY_NO_ERROR(); + CORRADE_VERIFY(shader.id() > 0); + CORRADE_COMPARE(shader.type(), Shader::Type::Fragment); + #ifndef MAGNUM_TARGET_GLES + CORRADE_COMPARE(shader.sources(), std::vector{"#version 130\n"}); + #else + CORRADE_COMPARE(shader.sources(), std::vector{"#version 300\n"}); + #endif + } + + MAGNUM_VERIFY_NO_ERROR(); +} + +void ShaderGLTest::constructCopy() { + CORRADE_VERIFY(!(std::is_constructible{})); + CORRADE_VERIFY(!(std::is_assignable{})); +} + +void ShaderGLTest::constructMove() { + #ifndef MAGNUM_TARGET_GLES + Shader a(Version::GL300, Shader::Type::Fragment); + #else + Shader a(Version::GLES300, Shader::Type::Fragment); + #endif + const Int id = a.id(); + + MAGNUM_VERIFY_NO_ERROR(); + CORRADE_VERIFY(id > 0); + + Shader b(std::move(a)); + + CORRADE_COMPARE(a.id(), 0); + CORRADE_COMPARE(b.id(), id); + CORRADE_COMPARE(b.type(), Shader::Type::Fragment); + #ifndef MAGNUM_TARGET_GLES + CORRADE_COMPARE(b.sources(), std::vector{"#version 130\n"}); + #else + CORRADE_COMPARE(b.sources(), std::vector{"#version 300\n"}); + #endif + + #ifndef MAGNUM_TARGET_GLES + Shader c(Version::GL210, Shader::Type::Vertex); + #else + Shader c(Version::GLES200, Shader::Type::Vertex); + #endif + const Int cId = c.id(); + c = std::move(b); + + MAGNUM_VERIFY_NO_ERROR(); + CORRADE_VERIFY(cId > 0); + CORRADE_COMPARE(b.id(), cId); + CORRADE_COMPARE(c.id(), id); + CORRADE_COMPARE(c.type(), Shader::Type::Fragment); + #ifndef MAGNUM_TARGET_GLES + CORRADE_COMPARE(c.sources(), std::vector{"#version 130\n"}); + #else + CORRADE_COMPARE(c.sources(), std::vector{"#version 300\n"}); + #endif } void ShaderGLTest::label() {