Skip to content

Commit

Permalink
GL: Cleanup std::string usage in Shader/AbstractShaderProgram
Browse files Browse the repository at this point in the history
  • Loading branch information
Hugo Amiard committed Nov 23, 2022
1 parent a496029 commit 464fad1
Show file tree
Hide file tree
Showing 9 changed files with 107 additions and 100 deletions.
37 changes: 20 additions & 17 deletions src/Magnum/GL/AbstractShaderProgram.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,14 @@

#include <Corrade/Containers/Array.h>
#include <Corrade/Containers/Iterable.h>
#include <Corrade/Containers/Pair.h>
#include <Corrade/Containers/StridedArrayView.h>
#ifndef MAGNUM_TARGET_WEBGL
#include <Corrade/Containers/String.h>
#endif
#include <Corrade/Containers/StringStl.h> /** @todo remove once <string>-free */
#include <Corrade/Containers/Reference.h>
#include <Corrade/Utility/Algorithms.h>
#include <Corrade/Utility/DebugStl.h>

#include "Magnum/GL/Context.h"
Expand Down Expand Up @@ -340,7 +342,7 @@ AbstractShaderProgram& AbstractShaderProgram::setLabel(const Containers::StringV
}
#endif

std::pair<bool, std::string> AbstractShaderProgram::validate() {
Containers::Pair<bool, Containers::String> AbstractShaderProgram::validate() {
glValidateProgram(_id);

/* Check validation status */
Expand All @@ -350,12 +352,11 @@ std::pair<bool, std::string> AbstractShaderProgram::validate() {

/* Error or warning message. The string is returned null-terminated, scrap
the \0 at the end afterwards */
std::string message(logLength, '\n');
Containers::String message(ValueInit, Math::max(logLength, 1)-1);
if(message.size() > 1)
glGetProgramInfoLog(_id, message.size(), nullptr, &message[0]);
message.resize(Math::max(logLength, 1)-1);

return {success, std::move(message)};
return {bool(success), std::move(message)};
}

AbstractShaderProgram& AbstractShaderProgram::draw(Mesh& mesh) {
Expand Down Expand Up @@ -554,32 +555,35 @@ void AbstractShaderProgram::bindFragmentDataLocationIndexedInternal(const Unsign
#endif

#ifndef MAGNUM_TARGET_GLES2
void AbstractShaderProgram::setTransformFeedbackOutputs(const Containers::ArrayView<const std::string> outputs, const TransformFeedbackBufferMode bufferMode) {
void AbstractShaderProgram::setTransformFeedbackOutputs(const Containers::ArrayView<const Containers::String> outputs, const TransformFeedbackBufferMode bufferMode) {
(this->*Context::current().state().shaderProgram.transformFeedbackVaryingsImplementation)(outputs, bufferMode);
}

void AbstractShaderProgram::setTransformFeedbackOutputs(const std::initializer_list<std::string> outputs, const TransformFeedbackBufferMode bufferMode) {
void AbstractShaderProgram::setTransformFeedbackOutputs(const std::initializer_list<Containers::String> outputs, const TransformFeedbackBufferMode bufferMode) {
setTransformFeedbackOutputs(Containers::arrayView(outputs), bufferMode);
}

void AbstractShaderProgram::transformFeedbackVaryingsImplementationDefault(const Containers::ArrayView<const std::string> outputs, const TransformFeedbackBufferMode bufferMode) {
void AbstractShaderProgram::transformFeedbackVaryingsImplementationDefault(const Containers::ArrayView<const Containers::String> outputs, const TransformFeedbackBufferMode bufferMode) {
/** @todo VLAs */
Containers::Array<const char*> names{outputs.size()};

Int i = 0;
for(const std::string& output: outputs) names[i++] = output.data();
for(const Containers::String& output: outputs) names[i++] = output.data();

glTransformFeedbackVaryings(_id, outputs.size(), names, GLenum(bufferMode));
}

#ifdef CORRADE_TARGET_WINDOWS
void AbstractShaderProgram::transformFeedbackVaryingsImplementationDanglingWorkaround(const Containers::ArrayView<const std::string> outputs, const TransformFeedbackBufferMode bufferMode) {
void AbstractShaderProgram::transformFeedbackVaryingsImplementationDanglingWorkaround(const Containers::ArrayView<const Containers::String> outputs, const TransformFeedbackBufferMode bufferMode) {
/* NVidia on Windows doesn't copy the names when calling
glTransformFeedbackVaryings() so it then fails at link time because the
char* are dangling. We have to do the copy on the engine side and keep
the values until link time (which can happen any time and multiple
times, so basically for the remaining lifetime of the shader program) */
_transformFeedbackVaryingNames.assign(outputs.begin(), outputs.end());
_transformFeedbackVaryingNames =
Containers::Array<Containers::String>{ValueInit, outputs.size()};
for(size_t i = 0; i < outputs.size(); ++i)
_transformFeedbackVaryingNames[i] = outputs[i];

transformFeedbackVaryingsImplementationDefault({_transformFeedbackVaryingNames.data(), _transformFeedbackVaryingNames.size()}, bufferMode);
}
Expand Down Expand Up @@ -608,10 +612,9 @@ bool AbstractShaderProgram::checkLink(const Containers::Iterable<Shader>& shader

/* Error or warning message. The string is returned null-terminated,
strip the \0 at the end afterwards. */
std::string message(logLength, '\n');
Containers::String message(ValueInit, Math::max(logLength, 1)-1);
if(message.size() > 1)
glGetProgramInfoLog(_id, message.size(), nullptr, &message[0]);
message.resize(Math::max(logLength, 1)-1);

/* Some drivers are chatty and can't keep shut when there's nothing to
be said, handle that as well. */
Expand Down Expand Up @@ -655,16 +658,16 @@ bool AbstractShaderProgram::isLinkFinished() {
return success == GL_TRUE;
}

void AbstractShaderProgram::cleanLogImplementationNoOp(std::string&) {}
void AbstractShaderProgram::cleanLogImplementationNoOp(Containers::String&) {}

#if defined(CORRADE_TARGET_WINDOWS) && !defined(MAGNUM_TARGET_GLES)
void AbstractShaderProgram::cleanLogImplementationIntelWindows(std::string& message) {
void AbstractShaderProgram::cleanLogImplementationIntelWindows(Containers::String& message) {
if(message == "No errors.\n") message = {};
}
#endif

#if defined(MAGNUM_TARGET_GLES) && !defined(MAGNUM_TARGET_WEBGL)
void AbstractShaderProgram::cleanLogImplementationAngle(std::string& message) {
void AbstractShaderProgram::cleanLogImplementationAngle(Containers::String& message) {
if(message == "\n") message = {};
}
#endif
Expand All @@ -676,15 +679,15 @@ void AbstractShaderProgram::completionStatusImplementationFallback(GLuint, GLenu
Int AbstractShaderProgram::uniformLocationInternal(const Containers::ArrayView<const char> name) {
const GLint location = glGetUniformLocation(_id, name);
if(location == -1)
Warning{} << "GL::AbstractShaderProgram: location of uniform \'" << Debug::nospace << std::string{name, name.size()} << Debug::nospace << "\' cannot be retrieved";
Warning{} << "GL::AbstractShaderProgram: location of uniform \'" << Debug::nospace << Containers::String{name, name.size()} << Debug::nospace << "\' cannot be retrieved";
return location;
}

#ifndef MAGNUM_TARGET_GLES2
UnsignedInt AbstractShaderProgram::uniformBlockIndexInternal(const Containers::ArrayView<const char> name) {
const GLuint index = glGetUniformBlockIndex(_id, name);
if(index == GL_INVALID_INDEX)
Warning{} << "GL::AbstractShaderProgram: index of uniform block \'" << Debug::nospace << std::string{name, name.size()} << Debug::nospace << "\' cannot be retrieved";
Warning{} << "GL::AbstractShaderProgram: index of uniform block \'" << Debug::nospace << Containers::String{name, name.size()} << Debug::nospace << "\' cannot be retrieved";
return index;
}
#endif
Expand Down
33 changes: 17 additions & 16 deletions src/Magnum/GL/AbstractShaderProgram.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,17 @@
* @brief Class @ref Magnum::GL::AbstractShaderProgram, macro @ref MAGNUM_GL_ABSTRACTSHADERPROGRAM_SUBCLASS_DRAW_IMPLEMENTATION(), @ref MAGNUM_GL_ABSTRACTSHADERPROGRAM_SUBCLASS_DISPATCH_IMPLEMENTATION()
*/

#include <string>
#include <Corrade/Containers/ArrayView.h>
#include <Corrade/Containers/String.h>
#include <Corrade/Containers/StringView.h>

#include "Magnum/Tags.h"
#include "Magnum/GL/AbstractObject.h"
#include "Magnum/GL/Attribute.h"
#include "Magnum/GL/GL.h"

#if defined(CORRADE_TARGET_WINDOWS) && !defined(MAGNUM_TARGET_GLES2)
#include <vector>
#include <Corrade/Containers/Array.h>
#endif

#ifdef MAGNUM_BUILD_DEPRECATED
Expand Down Expand Up @@ -832,7 +833,7 @@ class MAGNUM_GL_EXPORT AbstractShaderProgram: public AbstractObject {
* @def_gl{VALIDATE_STATUS}, @def_gl{INFO_LOG_LENGTH},
* @fn_gl_keyword{GetProgramInfoLog}
*/
std::pair<bool, std::string> validate();
Containers::Pair<bool, Containers::String> validate();

/**
* @brief Draw a mesh
Expand Down Expand Up @@ -1433,7 +1434,7 @@ class MAGNUM_GL_EXPORT AbstractShaderProgram: public AbstractObject {
* @ref GL-AbstractShaderProgram-attribute-location "class documentation"
* for more information.
*/
void bindAttributeLocation(UnsignedInt location, const std::string& name) {
void bindAttributeLocation(UnsignedInt location, Containers::StringView name) {
bindAttributeLocationInternal(location, {name.data(), name.size()});
}

Expand Down Expand Up @@ -1462,7 +1463,7 @@ class MAGNUM_GL_EXPORT AbstractShaderProgram: public AbstractObject {
* @requires_gl Multiple blend function inputs are not available in
* OpenGL ES or WebGL.
*/
void bindFragmentDataLocationIndexed(UnsignedInt location, UnsignedInt index, const std::string& name) {
void bindFragmentDataLocationIndexed(UnsignedInt location, UnsignedInt index, Containers::StringView name) {
bindFragmentDataLocationIndexedInternal(location, index, {name.data(), name.size()});
}

Expand All @@ -1489,7 +1490,7 @@ class MAGNUM_GL_EXPORT AbstractShaderProgram: public AbstractObject {
* in OpenGL ES 2.0 and @webgl_extension{WEBGL,draw_buffers} in
* WebGL 1.0.
*/
void bindFragmentDataLocation(UnsignedInt location, const std::string& name) {
void bindFragmentDataLocation(UnsignedInt location, Containers::StringView name) {
bindFragmentDataLocationInternal(location, {name.data(), name.size()});
}

Expand Down Expand Up @@ -1532,8 +1533,8 @@ class MAGNUM_GL_EXPORT AbstractShaderProgram: public AbstractObject {
* @requires_gl Special output names `gl_NextBuffer` and
* `gl_SkipComponents#` are not available in OpenGL ES or WebGL.
*/
void setTransformFeedbackOutputs(Containers::ArrayView<const std::string> outputs, TransformFeedbackBufferMode bufferMode);
void setTransformFeedbackOutputs(std::initializer_list<std::string> outputs, TransformFeedbackBufferMode bufferMode); /**< @overload */
void setTransformFeedbackOutputs(Containers::ArrayView<const Containers::String> outputs, TransformFeedbackBufferMode bufferMode);
void setTransformFeedbackOutputs(std::initializer_list<Containers::String> outputs, TransformFeedbackBufferMode bufferMode); /**< @overload */
#endif

/**
Expand Down Expand Up @@ -1601,7 +1602,7 @@ class MAGNUM_GL_EXPORT AbstractShaderProgram: public AbstractObject {
* @ref GL-AbstractShaderProgram-uniform-location "class documentation"
* for more information.
*/
Int uniformLocation(const std::string& name) {
Int uniformLocation(Containers::StringView name) {
return uniformLocationInternal({name.data(), name.size()});
}

Expand All @@ -1627,7 +1628,7 @@ class MAGNUM_GL_EXPORT AbstractShaderProgram: public AbstractObject {
* @ref GL-AbstractShaderProgram-uniform-block-binding "class documentation"
* for more information.
*/
UnsignedInt uniformBlockIndex(const std::string& name) {
UnsignedInt uniformBlockIndex(Containers::StringView name) {
return uniformBlockIndexInternal({name.data(), name.size()});
}

Expand Down Expand Up @@ -1920,18 +1921,18 @@ class MAGNUM_GL_EXPORT AbstractShaderProgram: public AbstractObject {
UnsignedInt uniformBlockIndexInternal(Containers::ArrayView<const char> name);

#ifndef MAGNUM_TARGET_GLES2
void MAGNUM_GL_LOCAL transformFeedbackVaryingsImplementationDefault(Containers::ArrayView<const std::string> outputs, TransformFeedbackBufferMode bufferMode);
void MAGNUM_GL_LOCAL transformFeedbackVaryingsImplementationDefault(Containers::ArrayView<const Containers::String> outputs, TransformFeedbackBufferMode bufferMode);
#ifdef CORRADE_TARGET_WINDOWS
void MAGNUM_GL_LOCAL transformFeedbackVaryingsImplementationDanglingWorkaround(Containers::ArrayView<const std::string> outputs, TransformFeedbackBufferMode bufferMode);
void MAGNUM_GL_LOCAL transformFeedbackVaryingsImplementationDanglingWorkaround(Containers::ArrayView<const Containers::String> outputs, TransformFeedbackBufferMode bufferMode);
#endif
#endif

static MAGNUM_GL_LOCAL void cleanLogImplementationNoOp(std::string& message);
static MAGNUM_GL_LOCAL void cleanLogImplementationNoOp(Containers::String& message);
#if defined(CORRADE_TARGET_WINDOWS) && !defined(MAGNUM_TARGET_GLES)
static MAGNUM_GL_LOCAL void cleanLogImplementationIntelWindows(std::string& message);
static MAGNUM_GL_LOCAL void cleanLogImplementationIntelWindows(Containers::String& message);
#endif
#if defined(MAGNUM_TARGET_GLES) && !defined(MAGNUM_TARGET_WEBGL)
static MAGNUM_GL_LOCAL void cleanLogImplementationAngle(std::string& message);
static MAGNUM_GL_LOCAL void cleanLogImplementationAngle(Containers::String& message);
#endif

MAGNUM_GL_LOCAL static void APIENTRY completionStatusImplementationFallback(GLuint, GLenum, GLint*);
Expand Down Expand Up @@ -2009,7 +2010,7 @@ class MAGNUM_GL_EXPORT AbstractShaderProgram: public AbstractObject {
#if defined(CORRADE_TARGET_WINDOWS) && !defined(MAGNUM_TARGET_GLES2)
/* Needed for the nv-windows-dangling-transform-feedback-varying-names
workaround */
std::vector<std::string> _transformFeedbackVaryingNames;
Containers::Array<Containers::String> _transformFeedbackVaryingNames;
#endif
};

Expand Down
4 changes: 2 additions & 2 deletions src/Magnum/GL/Implementation/ShaderProgramState.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ struct ShaderProgramState {
void reset();

#ifndef MAGNUM_TARGET_GLES2
void(AbstractShaderProgram::*transformFeedbackVaryingsImplementation)(Containers::ArrayView<const std::string>, AbstractShaderProgram::TransformFeedbackBufferMode);
void(AbstractShaderProgram::*transformFeedbackVaryingsImplementation)(Containers::ArrayView<const Containers::String>, AbstractShaderProgram::TransformFeedbackBufferMode);
#endif
void(*cleanLogImplementation)(std::string&);
void(*cleanLogImplementation)(Containers::String&);
/* This is a direct pointer to a GL function, so needs a __stdcall on
Windows to compile properly on 32 bits */
void(APIENTRY *completionStatusImplementation)(GLuint, GLenum, GLint* value);
Expand Down
4 changes: 2 additions & 2 deletions src/Magnum/GL/Implementation/ShaderState.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ struct ShaderState {
#endif
};

void(Shader::*addSourceImplementation)(std::string);
void(*cleanLogImplementation)(std::string&);
void(Shader::*addSourceImplementation)(Containers::StringView);
void(*cleanLogImplementation)(Containers::String&);
/* This is a direct pointer to a GL function, so needs a __stdcall on
Windows to compile properly on 32 bits */
void(APIENTRY *completionStatusImplementation)(GLuint, GLenum, GLint* value);
Expand Down
Loading

0 comments on commit 464fad1

Please sign in to comment.