OpenGL shader class

I'm working my way through some basic OpenGL tutorials and have decided to offload the shader loading/compiling/linking to a separate object that I'll use for the remainder of the tutorial material. Ideally, I'd also like it to be useful later, when I get on with working my own OpenGL projects. And, who knows, maybe someone else would find it useful.

That said, I'd like someone (everyone?) to offer their feedback on the robustness and correctness of this code. I'm shooting for C++11/14 modes and methods as well as useful errors on something going wrong.

One thing I know doesn't work correctly is that if the user tries to create an instance of a Shader object using the constructor that takes file names before they've created an OpenGL context, it simply segfaults. I'm not sure how I can stop OpenGL from doing that, though.

And, of course, if anyone sees any needless complexity, poor practices, unused includes, etc, I'd be happy to have those pointed out too. =)

Header:

```
#ifndef SHADER_H
#define SHADER_H

#include
#include
#include
#include
#include

using std::string;
using std::ifstream;
using std::cerr;
using std::endl;
using std::invalid_argument;
using std::runtime_error;
using std::ios;

#include

class Shader
{
private:
GLuint program = 0, vertex_shader = 0, fragment_shader = 0;
GLchar vertex_shader_source = nullptr, fragment_shader_source = nullptr;
GLint vertex_shader_status = GL_FALSE, fragment_shader_status = GL_FALSE,
shader_program_status = GL_FALSE;

GLenum gl_error = GL_NO_ERROR;

void load_shader(const std::string &filename, GLchar *&shader_source);
void shader_compile_error(const GLuint &vertex_shader,
const string &vertex_source,
const GLuint &fragment_shader,
const string &fragment_source);
void shader_link_error(const GLuint &shader_program);

public:
Shader();
Shader(const string &vertex_filename, const string &fragment_

A few general comments

Overall, your code has a very "90's" look-n-feel to it. I say that because old codebases and your code tend to have two things in common:

• Heavy use of raw pointers and manual memory management (new/delete).

• (ab)use of try/catch blocks.

Number one is a no-brainer to fix. Get familiar with smart pointers and modern resource management in C++. The overall concept behind this is called RAII - Resource Acquisition Is Initialization. Resource lifetime management should be automated, as much as possible.

The second issue gets partly solved by RAII, the need to delete those pointers will be gone, and so will half of your try/catch blocks. But also to get a better grasp of exception usage and handling in C++, I recommend taking some time to watch this presentation when you have the chance: “The Unexceptional Exceptions".

Specifics

This is bad, specially in a header file:

using std::string;
using std::ifstream;
using std::cerr;
using std::endl;
using std::invalid_argument;
using std::runtime_error;
using std::ios;

You don't want to force users of your code to import all those names into the global namespace. Say I have a class named string in my own code, if I include your header file, havoc will ensue. std:: qualify your uses of the Standard library instead, it will make your code more robust. Related: Why is “using namespace std;” considered bad practice?

Easy on the statefull objects. The less transient state you keep around inside your classes, the better. Not only it will save memory, but also make your code easier to reason about. Mutable state is a huge source of bugs, so force yourself to pass stuff around as function parameters as much as you can and slap const in front of the rest.

It's very unlikely that keeping the gl_error around will have any use once the error is printed. Same for the shader compile statuses. You can infer if the shaders are valid by simply checking that the handle is nonzero. I would also try to avoid keeping copies of the shader source code around. My GL shader classes are usually comprised of just the program handle. The shader handles themselves are pretty useless once attached to a program, so you can even dispose of them after shader compilation.


One thing I know doesn't work correctly is that if the user tries to create an instance of a Shader object using the constructor that takes file names before they've created an OpenGL context, it simply segfaults.

Easy fix by making dependencies explicit. Define a GLContext class that users have to instantiate somehow (probably by passing it some window handle that can only be acquired after creating a hardware context).

Shaders must then receive a GLContext instance upon creation, which you can then assert that it is valid.

Other minutiae:

-
Empty constructor could be removed or set to = default; in the declaration.

-
You can make your exception throwing code one-liners: throw ExceptionType("message");. No need for the local exs all around.

-
You should probably delete the copy constructor and assignment operator, but your class could be movable. Read about the Rule of 3/5/0.

StackExchange Code Review Q#118461, answer score: 8