(Ab)using constructors and destructors for side effects bad practice? Alternatives?

In OpenGL, one often writes code like this:

glPushMatrix();
// modify the current matrix and use it
glPopMatrix();

Essentially, the state is changed, then some actions are performed that use the new state, and finally the state is restored.

Now there are two problems here:

1. It's easy to forget to restore the state.
2. If the code in between throws an exception, the state is never restored.

In true object-based programming style, I have written some utility classes to overcome these problems, like so:

struct WithPushedMatrix {
    WithPushedMatrix() { glPushMatrix(); }
    ~WithPushedMatrix() { glPopMatrix(); }
};

Now I can simply write my previous example like this:

WithPushedMatrix p;
// modify the current matrix and use it

The exact moment of restoring is determined by the lifetime of p. If an exception is thrown, p's destructor gets called, the state is restored, and life is good.

Still, I'm not entirely happy. Especially if the constructor takes some argu开发者_StackOverflow社区ments (e.g. flags for glEnable), it's easy to forget to assign the object to a variable:

WithEnabledFlags(GL_BLEND); // whoops!

The temporary gets destroyed immediately, and the state change is reversed prematurely.

Another issue is that anyone else reading my code might get confused: "Why is there a variable declared here that is never used? Let's get rid of it!"

So, my questions: Is this a good pattern? Does it maybe even have a name? Are there any problems with this approach that I'm overlooking? Last but not least: are there any good alternatives?

Update: Yes, I guess it's a form of RAII. But not in the way RAII is normally used, because it involves a seemingly useless variable; the "resource" in question is never accessed explicitly. I just didn't realize that this particular usage was so common.

---

I like the idea of using RAII to control OpenGL state, but I'd actually take it one step further: have your WithFoo class constructor take a function pointer as a parameter, which contains the code you want to execute in that context. Then don't create named variables, and just work with temporaries, passing in the action you want to execute in that context as a lambda. (needs C++0x, of course - can work with regular function pointers too but it's not nearly as pretty.)
Something like this: (edited to restore exception-safety)

class WithPushedMatrix
{
public:
    WithPushedMatrix()
    {
        glPushMatrix();
    }

    ~WithPushedMatrix()
    {
        glPopMatrix();
    }

    template <typename Func>
    void Execute(Func action)
    {
        action();
    }
};

And use it like so:

WithPushedMatrix().Execute([]
{
    glBegin(GL_LINES);
    //etc. etc.
});

The temporary object will set up your state, execute the action and then tear it down automatically; you don't have "loose" state variables floating around, and the actions executing under the context become strongly associated with it. You can even nest multiple contextual actions without worrying about destructor order.

You can even take this further and make a generic WithContext class that takes additional setup and teardown function parameters.

edit: Had to move the action() call into a separate Execute function to restore exception-safety - if it's called in the constructor and throws, the destructor won't get called.

edit2: Generic technique -

So I fiddled around with this idea some more, and came up with something better:
I'll define a With class, that creates the context variable and stuffs it into a std::auto_ptr in it's initializer, then calls the action:

template <typename T>
class With
{
public:
    template <typename Func>
    With(Func action) : context(new T()) 
    { action(); }

    template <typename Func, typename Arg>
    With(Arg arg, Func action) : context(new T(arg))
    { action(); }

private:
    const std::auto_ptr<T> context;
};

Now you can combine it with context type that you defined originally:

struct PushedMatrix 
{
    PushedMatrix() { glPushMatrix(); }
    ~PushedMatrix() { glPopMatrix(); }
};

And use it like this:

With<PushedMatrix>([]
{
    glBegin(GL_LINES);
    //etc. etc.
});

or

With<EnabledFlag>(GL_BLEND, []
{
    //...
});

Benefits:

1. Exception-safety is handled by the auto_ptr now, so if action throws, the context will still get destroyed properly.
2. No more need for an Execute method, so it looks clean again! :)
3. Your "context" classes are dead-simple; all of the logic is handled by the With class so you just need to define a simple ctor/dtor for each new type of context.

One niggle: As I've written it above, you need to declare manual overloads for the ctor for as many parameters as you need; although even just one should cover most OpenGL use cases, this isn't really nice. This should be neatly fixed with variadic templates - just replace typename Arg in the ctor with typename ...Args - but it'll depend on compiler support for that (MSVC2010 doesn't have them yet).

---

Using objects like that is called RAII and is very typical for resource management. Yes, sometimes you'll have temporary objects destroyed too early because you forgot to provide a varible name. But you have one big advantage here - code becomes more exception safe and cleaner - you don't have to call all the cleanup stuff manually on all possible code paths.

One advice: use reasonable variable names, not p. Call it matrixSwitcher or something like that, so that readers don't think it's a useless variable.

---

As was pointed out by others, this is a well-known and encouraged pattern in C++.

A way to deal with the problem of forgetting the variable name is to define operations so that they need the variable. Either by making the possible actions a member of the RAII class:

PushedMatrix pushed_matrix;;
pushed_matrix.transform( /*...*/ );

or by making functions take the RAII class as an argument:

PushedMatrix pushed_matrix;
transform_matrix( pushed_matrix, /*...*/ );

---

I'd like to point out, that my answer actually contains useful information (more that a vague reference to RAII which is apparently 19 upvotes worth). It does not need c++0x to work, is not hypothetical at all and fixes the OP's problems which relate to the need to declare a variable.

---

There's a very nice way to beef up RAII constructs (or more precisly: ScopeGuards) syntactically: the if() statement accepts declarations which are scoped to the if-block:

#include <stdio.h>

class Lock
{
    public:
    Lock() { printf("locking\n"); }
    ~Lock() { printf("unlocking\n"); }
    operator bool () const { return true;}
};
int main()
{
    // id__ is valid in the if-block only
    if (Lock id_=Lock()) {  
        printf("..action\n");
    }
}

this prints:

locking
..action
unlocking

If we add a bit of syntactic sugar, we can write

#define WITH(X) if (X with_id_=X())
int main()
{
    WITH(Lock) {    
        printf("..action\n");
        WITH(Lock) {
            printf("more action\n");
        }
    }
}

And now we use the fact that temporaries which are used to initialize a const reference remain alive as long as the const reference stays in scope, to make it work with parameters (We also fix the nuisance that WITH(X) accepts a trailing else):

   #include <stdio.h>
   class ScopeGuard 
   {
    public:
    mutable int dummy;
    operator bool () const { return false;}
    ScopeGuard(){}
    private:
    ScopeGuard(const ScopeGuard &); 
    }; 
    class Lock : public ScopeGuard
    {
        const char *s;
        public: 
        Lock(const char *s_) : s(s_) { printf("locking %s\n",s); }
        ~Lock() { printf("unlocking %s\n",s); }
    };

    #define WITH(X) if (const ScopeGuard& with_id_=X)  {} else 
    int main()
    {
        WITH(Lock("door")) {    
            printf("..action\n");
            WITH(Lock("gate")) {
                printf("more action\n");
            }
        }
    }

TATA!

A nice side effect of this method is, that all "protected" regions are uniformly identified through the WITH(...) {...} pattern - a nice property for code-reviews et.al.

---

Warning: C++0x-oriented answer

The pattern you're using is RAII and it's widely used for resource management. The only possible alternative is to use try-catch blocks, but it usually makes your code a bit too messy.

Now the problems. First if you don't want to code a different class for each combinaison of OpenGL functions, there is another benefit of C++0x which is that you can write lambda functions and store them in a variable. So if I was you, I would create a class like this:

template<typename Destr>
class MyCustom {
public:
    template<typename T>
    MyCustom(T onBuild, Destr onDestroy) : 
        _onDestroy(std::move(onDestroy))
    {
        onBuild();
    }

    ~MyCustom() { _onDestroy(); }

private:
    Destr    _onDestroy;
};

template<typename T1, typename T2>
MyCustom<T2> buildCustom(T1 build, T2 destruct)   { return MyCustom<T2>(std::move(build), std::move(destruct)); }

Then you can use it like this:

auto matrixPushed = buildCustom([]() { glPushMatrix(); }, []() { glPopMatrix(); });

Or even better here:

auto matrixPushed = buildCustom(&glPushMatrix, &glPopMatrix);

This would also solve the problem of "why is this useless variable here", since its purpose now becomes obvious.

The function passed to the constructor should be inlined, so there is no performance overhead. The destructor should be stored like a function pointer, since lambda functions with nothing inside the brackets [] should be implemented like plain functions (according to the standards).

Your "variable instantly destroyed" problem would also be partially solved using "buildCustom" since you can see more easily where you forgot the variable.

---

To help you understand how long c++ programmers have been doing this, I learned about this technique in the late 90's working with COM.

I think it's a personal choice as to the exact mechanism you use to leverage the fundamental properties of c++ stack frames and destructors to make your object lifetime management easier. I wouldn't go very far out of my way to avoid needing to assign to a variable.

(this next thing I'm not 100% sure about but I'm putting it in hopes that someone will chime in - I know I've done this in the past but I couldn't find it in Google just now and I've been trying to remember... see, garbage collectors have dulled my mind!)

I believe you can force the scope with a plain old pair of curlys (POPOC).

{ // new stack frame
  auto_ptr<C> instanceA(new C);
  {
     auto_ptr<C> instanceB(new C);
  }
  // instanceB is gone
} 
// instanceA is gone

---

This is typical RAII example. Disadvantage of this method is appearing of many additional classes. To solve this problem you may create generic "guard" class if it is possible. There are another alternative: boost "Scope Exit" library (http://www.boost.org/doc/libs/1_43_0/libs/scope_exit/doc/html/index.html). You may try it, if you can depend on boost of course.

---

ScopeGuard comes to mind. Please note that with C++0x bind and variadic templates it may be rewritten to be much shorter.

---

I have never seen this before, but I must admit, it is somewhat cool. But I wouldn't use it, as it is not really intuitive.

EDIT: As sharptooth pointed out, this is called RAII. The example I found on wikipedia wraps also the operations on the resource in method calls. In your example, this would look like the following:

WithPushedMatrix p;
p.setFLag(GL_BLEND);
p.doSomething();

Then it is clear for what the variable is, and other developers will get the intuition if they read your code. Of course, the OpenGL code is then hidden, but I think one gets used to it really fast.

---

I think it is great and Idiomatic C++. The downside is that you basically write a (custom) Wrapper around the C OpenGL library. It would be great if such a Library existed, maybe something like an (semi-)official OpenGL++ lib. That said, I have written code like this (from Memory), and have been very happy with it:

{
  Lighting light = Light(Color(128,128,128));
    light.pos(0.0, 1.0, 1.0);
  Texture tex1 = Texture(GL_TEXTURE1);
    tex1.set(Image("CoolTex.png"));

  drawObject();
}

The overhead in writing the wrappers in not very onerous, and the resulting code is as good as the handwritten code. And is IMHO much easier to read than the corresponding OpenGL code, even if you do not know the wrappers by heart.