Where objects created when passing parameters should be deleted?

Question

I have something like this:

class GLSLShader {
    public:
        GLSLShader(GLenum);
        ~GLSLShader();
};

class GLSLProgram {
    public:
        GLSLProgram(GLSLShader *, GLSLShader *);
        ~GLSLProgram();

    private:
        GLSLShader *m_VertexShader;
        GLSLShader *m_FragmentShader;
};

GLSLProgram constructor is going to be called like this:

GLSLProgram program(new GLSLShader(GL_VERTEX_SHADER), new GLSLShader(GL_FRAGMENT_SHADER));

My question is where should I delete the allocated Shader objects. Should I delete it on GLSLProgram's destructor or should I manage it differently with something like the code below?

GLSLShader *vertex = new GLSLShader(GL_VERTEX_SHADER);
GLSLShader *fragment = new GLSLShader(GL_FRAGMENT_SHADER);
GLSLProgram program(vertex, fragment);
delete vertex;
delete fragment;

[Use RAII](http://stackoverflow.com/questions/395123/raii-and-smart-pointers-in-c) — Rakete1111, Jun 28 '16 at 13:40
@Rakete1111: He is using RAII. `GLSLShader`'s destructor will clean up the resources that the constructor allocates. He's just using RAII *really badly* — Nicol Bolas, Jun 28 '16 at 13:47
It is a question of **ownership**. If you suppose GLSLProgram to be an owner of this objects, it should delete them on destruction. If you suppose that GLSLProgram just operates with them, it shouldn't. — ilotXXI, Jun 28 '16 at 13:54

Nicol Bolas · Accepted Answer · 2016-06-28T13:59:12.283

You have two basic problems.

The first problem is that you are using RAII objects incorrectly. The point of RAII objects is to manage object lifetimes using automatic variables, not by using new and delete explicitly. By using new on GLSLShader, without wrapping that pointer into a RAII container, you're effectively mitigating the work your destructors do.

Your GLSLProgram constructor should not take the objects by pointer. It should take them by const&. This makes it easier for the caller to decide how long those objects should last.

The second problem is logical. There is no reason for GLSLProgram to be responsible for the destruction of the shader objects it is given. That should be up to the calling code, since GLSL shader objects can be reused.

As such, GLSLProgram should not attempt to destroy those objects. But after successfully creating the program, the constructor should detach the GLSL shaders from the GLSL program with glDetachShader.

Overall, your code ought to look like this:

GLSLShader vertex(GL_VERTEX_SHADER);
GLSLShader fragment(GL_FRAGMENT_SHADER);
//Fill `vertex` and `fragment` with actual shader code.
GLSLProgram program(vertex, fragment);
//Destructors for `vertex` and `fragment` will take care of themselves.

Also, you should be aware of a few other things:

Programs can be created with more than 2 shader objects. So you should have alternate constructors that can take many such objects.
Separable programs can be created directly from strings, without using shader objects at all.

It should be expected that I was using RAII incorrectly since I was not aware of its existence. I was in doubt if `GLSLProgram` should be responsible or not for the destruction of the shader since, as you said, they can be reused. But another question, `GLSLProgram` should take them by `const&` but the member variables should stay pointers? — Jean Catanho, Jun 28 '16 at 14:05
And about the other things, I'm aware of both of that situations. I just didn't get to the point of implementing it yet. — Jean Catanho, Jun 28 '16 at 14:08
@JeanCatanho: "*the member variables should stay pointers*" There should not be member variables for shaders; the program object shouldn't *keep them* at all. It should use them to link the program it creates, detach the shader after successfully creating the program, and exit. After the constructor has done its job, it no longer has need of them. — Nicol Bolas, Jun 28 '16 at 14:25

eerorika · Answer 2 · 2016-06-28T14:05:23.380

Should I delete it on GLSLProgram's destructor or should I manage it differently

It depends.

Does anything other than GLSLProgram need access to those dynamic objects? Can such objects outlive the GLSLProgram instance that points to them? If yes to both, then the memory can not be managed solely by program.

In either case, you'll find that it is much easier to write a correct program, if you delegate the memory management to a RAII object that is responsible only of destruction of the dynamic object.

On the other hand, if the GLSLProgram can manage the memory solely, then surely it should also be then one to allocate them. Furthermore, there seems to be no reason to use dynamic allocation at all. I recommend using member objects instead, unless there is a specific reason not to.

Given the attempt in the question, I suggest following:

class GLSLProgram {
    public:
        GLSLProgram():
            m_VertexShader(GL_VERTEX_SHADER),
            m_FragmentShader(GL_FRAGMENT_SHADER) {}

    private:
        GLSLShader m_VertexShader;
        GLSLShader m_FragmentShader;
};

Since I was not aware of RAII, I didn't know exactly how to handle this but, as you said, maybe there's no reason to use dynamic allocation at all here even if GLSLProgram can't manage the memory solely. — Jean Catanho, Jun 28 '16 at 14:22

Where objects created when passing parameters should be deleted?

2 Answers2