Yet another OpenGL Shader: the Builder pattern and lifetime management

I've noticed that whenever a shader program class is implemented (from some questions on this site, as well as more reputable implementations), that:

-
The class tries to do too much. Binding to the current context, setting attribute variable values, and much more.

-
Use the JavaBeans pattern in one form or another, allowing the shader program to be in an invalid or incomplete state. For example, in between addShaderFromSource calls, the program is only partially built, and not usable.

My ideal shader program class would do much less; its purpose would just be to manage the lifetime of a shader program object, while maintaining a valid state throughout the object's lifetime (either a zero program or a valid, linked program), and the solution I came up with to achieve this uses the builder pattern.

```
void deleteAttachedShaders(GLuint programId) {
constexpr GLsizei MAX_SHADER_COUNT = 5;
GLuint shaderIds[MAX_SHADER_COUNT];
GLsizei shaderActualCount;
glGetAttachedShaders(programId, MAX_SHADER_COUNT, &shaderActualCount, shaderIds);

for(int i = 0; i fileBegin(fileName), fileEnd;
std::string fileContents(fileBegin, fileEnd);
return addShaderFromSource(fileContents, shaderType);
}

GLShaderProgram linkProgram() {
glLinkProgram(programId_);
GLShaderProgram theProgram(programId_);
deleteAttachedShaders(programId_);
linked_ = true;
return theProgram;
}

~Builder() {
if(!linked_) {
glDeleteProgram(programId_);
}
}
private:
GLuint programId_;
bool linked_;
};

GLShaderProgram() : programId_(0) { }

GLShaderProgram(GLShaderProgram &&other) {
*this = std::move(other);
}

GLShaderProgram &operator=(GLShaderProgram &&other)
{
programId_ = other.programId_;
other.programId_ = 0;

if (other.programId_ != 0) {

Your implementation is very OOPy and I'd call it a good rendering of the builder pattern. Your usage example makes for a nice "fluent" API, BUT, I would personally not buy it.

The reason why 99% of the implementations out there are just reinventing OpenGL in a OOP way is because everyone takes the sorter path of a bottom up approach. They look at the API and see: Humm, so it looks like we have this glShaderSource thingy, so my class will need an addShaderSource method...

If they'd take a top down approach instead, they would first consider if exposing an addShaderSource method of sorts is even necessary.

-
First basic constraint of a GL shader program is that it requires at least two stages: Vertex and Fragment. So it doesn't even make sense for a shader program to exist without these two stages.

-
Linking the program is also a low-level implementation detail that I'd avoid exposing to the interface whenever possible. Again, a shader that fails to link is useless and an unlinked object should not be allowed to propagate.

Take all that into account and what should come to you is the good ole factory function. It receives all the required inputs and return a valid object or fails in your favourite way (return null, exception, error code, etc) without leaking intermediate incomplete objects.

// 
// Creates and links the shader or returns a null to indicate failure.
// You could also do a lot more for error handling, like throwing and exception
// with the compiler info log or returning the info log as an optional output parameter.
// I usually integrate this kind of stuff with a log system, so that's where my shader info log goes.
//
std::unique_ptr createGLShaderProgram(const std::string & vertexFilename, const std::string & fragmentFilename);

Now suppose you'd like to support more shader stages rather then the minimal Vertex+Fragment pair, like Geometry shader or Compute shaders. There are many ways to do it, but sticking to our previous example, I'd personally introduce a helper structure in this case:

struct ProgramStage
{
    enum Type
    {
        Vertex,
        Fragment,
        Geometry,
        // ... what have you ...
    };

    Type type;
    std::string filename; // This could also be a source code string instead if it suits you.
};

std::unique_ptr createGLShaderProgram(const std::vector & stages);

I hope the above snippets made the idea clear to you. The point is not to just try to wrap OpenGL in a class, but take a step back and actually consider what is really needed, which is avoiding the propagation of incomplete objects (I'd also add to that a simple and clean interface!). Your builder pattern doesn't do that. If the user forgets to add a mandatory stage, BAM!, incomplete state.

Now lets look at the rest of the code.

Move operator is broken:

GLShaderProgram &operator=(GLShaderProgram &&other)
{
    programId_ = other.programId_;
    other.programId_ = 0;

    if (other.programId_ != 0) {
        glDeleteProgram(other.programId_);
    }

    return *this;
}

Look closely to it in isolation for a few seconds. Do you realize the bug? other.programId_ is set to zero, then you test if it is non-zero in the next line to delete it. That if check is always false. But there is another less obvious error there. programId_ from this object gets overwritten, so you are officially leaking that handle if it contained a valid object.

You'd better just scrap that operator and rewrite a new one using the copy and swap idiom.

glDeleteShader != glDeleteProgram:

~GLShaderProgram() {
    glDeleteShader(programId_);
}

I'm fairly confident that the member programId_ is a shader program, not a shader object/stage, so it is being deleted by the wrong function. This is undefined behavior at best. You should be using glDeleteProgram, that was probably a typo, since in other places you use the correct function. Blame OpenGL for using goddamned integers for everything!

Make deleteAttachedShaders a member of something?

If that function is not useful elsewhere, then it should be made a static member of one of your classes.

StackExchange Code Review Q#97257, answer score: 7