Method manipulates global variables, mark const?

Question

I wrote a class ShaderWrapper in C++, that wraps around OpenGL shaders. In the constructor I generate the actually OpenGL shader object, but this method only returns me a handle/id of type int. The actuall shader object is stored somewhere in global space. So my ShaderWrapper objects only have to store one integer each.

In this class I have methods, that look like this:

void ShaderWrapper::setFloat(char* name, float value)
{
    glUniform1f(glGetUniformLocation(shaderID, name), value);
}

This method doesn't actually changes any data in the ShaderWrapper object, but it changes some data in the OpenGL shader object.

My IDE (Visual Studio + Resharper) suggests, that this method should be marked as const. Is this a good style? Marking it as const feels weird, since I actually change some data, just not in the wrapper object itself, but in the hidden OpenGL object.

Answer 1

Keep it non-const.

I assume that for the users of ShaderWrapper the underlying OpenGL object is an implementation detail they don’t need and want to be concerned with. So from the user’s point of view setFloat() does change the observable state of a ShaderWrapper object. That’s why it shouldn’t be const even though technically it could be.

Where you should consider constness is the name parameter. Any particular reason it cannot be a const char*? It doesn’t look like that char array gets modified anywhere, so const seems appropriate. It would also allow you to pass a string literal to setFloat(), which is impossible at the moment.

Food for thought

(with the caveat that I don’t know OpenGL at all)

It seems to me what you really care about is a combination of a shader and some named property, because that’s the entity you do actual work on.

You could introduce a Location accordingly and change ShaderWrapper so that it’s purpose is to be a resource manager for an OpenGL shader object and a factory for Location objects. For the real work you then have free-standing functions that take a Location& or const Location& as a parameter.

That’s a bit like the container plus iterator approach in that Location objects “index” into a shader and are invalidated once their ShaderWrapper dies. Obviously that creates the problem of possibly dangling Location objects. So maybe a ShaderID needs to become a shared_ptr like thing that manages shared ownership and kills the OpenGL object once its last Location dies.

Obviously all of that is just a rough round of brainstroming. The point is: I see here the temptation to create a “God class” and the opportunity to resist that temptation and create a possibly more elegant, safer and easier to use design that combines several small pieces with very specific purposes instead. Or at the end you’ll have understood exactly why you want that big powerful single class in this situation.

Answer 2

Logically you're mutating the shader, it doesn't matter that you outsourced actual storage to a global lookup table. So this method should not be callable on a constant view of the shader and the method should not be const.

You might want to add a wrapper for location as well, so you can model a mutable and constant location reference and then only support getting a mutable location on a mutable shader object. This minimizes the amount of code affected by the mismatch between C++'s const semantics and the OpenGL API.