1
\$\begingroup\$

This is a fixed up version (following RAII principles) of my previous reviewal. I have moved the construction of the object entirely to the constructor to prevent any bad object states.

The only thing I am slightly concerned about is if an exception is thrown, the destructor will not be called thus causing shader objects to leak (as glDeleteShader won't be called).

Maybe wrapping the shader handle ID (shader_id) in some sort of POD-wrapper could fix this?

compilation_error.h

#pragma once

#include <stdexcept>
#include <string>

class CompilationError : public std::runtime_error {
public:
    CompilationError(const std::string &message, std::string info_log);

    const std::string &get_info_log() const;

private:
    const std::string info_log;
};

compilation_error.cpp

#include <render/shader/compilation_error.h>

CompilationError::CompilationError(const std::string &message, std::string info_log) :
    runtime_error(message), info_log(std::move(info_log)) {}

const std::string &CompilationError::get_info_log() const {
    return info_log;
}

shader.h

#pragma once

#include <engine.h>
#include <string>

class Shader {
public:
    Shader(GLenum type, const std::string &path);

    ~Shader();

private:
    const GLuint shader_id;
};

shader.cpp

#include <render/shader/shader.h>
#include <render/shader/compilation_error.h>
#include <fstream>

Shader::Shader(const GLenum type, const std::string &path) :
    shader_id(glCreateShader(type)) {
    if (!shader_id) {
        throw std::runtime_error("Unable to create shader");
    }

    std::ifstream input_stream(path, std::ifstream::in | std::ifstream::ate);

    if (!input_stream) {
        glDeleteShader(shader_id);

        throw std::ifstream::failure("Unable to open shader file");
    }

    const long source_length = input_stream.tellg();
    std::vector<char> source((unsigned long) source_length);

    input_stream.seekg(0);
    input_stream.read(source.data(), source_length);

    const GLchar *sources[] = {source.data()};
    const GLint lengths[] = {(GLint) source_length};

    glShaderSource(shader_id, 1, sources, lengths);
    glCompileShader(shader_id);

    GLint compile_status;
    glGetShaderiv(shader_id, GL_COMPILE_STATUS, &compile_status);

    if (!compile_status) {
        GLint log_length;
        glGetShaderiv(shader_id, GL_INFO_LOG_LENGTH, &log_length);

        std::vector<GLchar> log_output((unsigned long) log_length);
        glGetShaderInfoLog(shader_id, log_length, nullptr, log_output.data());

        std::string info_log(log_output.data());

        if (!info_log.empty() && info_log.back() == '\n') {
            info_log.pop_back();
        }

        glDeleteShader(shader_id);

        throw CompilationError("Unable to compile shader", info_log);
    }
}

Shader::~Shader() {
    glDeleteShader(shader_id);
}
\$\endgroup\$
1
\$\begingroup\$

This looks a lot better! Good job!

Nitty-gritty stuff:

better cleanup in the constructor

You hava a lot of the following idiom in the constructor:

if(condition) {
  glDeleteShader(shader_id);
  throw something;
}

This is a little fragile, since should you make the class more complex, that's a lot of places to refactor. The following would be better:

try {
  //most of the constructor in here
  ...
  if(condition) throw something;
  ...
}
catch(...) {
  glDeleteShader(shader_id);
  throw;
}

ifstream can throw its own exceptions if you tell it to

ifstream input_stream;
input_stream.exceptions( std::ios::failbit );
input_stream.open(path, std::ifstream::in | std::ifstream::ate);

Use iterators when converting a vector to a string

std::string info_log(log_output.data());
vs
std::string info_log(log_output.begin(), log_output.end());

The second one is better because it works whether there is a null terminator or not. It also removes a strlen() call from the code, which is a O(N) operation.

That's actually all I've got. Good job once again.

\$\endgroup\$
  • \$\begingroup\$ Thank you again, you're great! It's all starting to make sense. \$\endgroup\$ – user148303 Sep 11 '17 at 23:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy