11

Since there is no finally in C++ you have to use the RAII design pattern instead, if you want your code to be exception safe. One way to do this is by using the destructor of a local class like this:

void foo() {
    struct Finally {
        ~Finally() { /* cleanup code */ }
    } finalizer();
    // ...code that might throw an exception...
}

This is a big advantage over the straight forward solution, because you don't have to write the cleanup code 2 times:

try {
    // ...code that might throw an exception...
    // cleanup code (no exception)
} catch (...) {
    // cleanup code (exception)
    throw;
}

A big disadvantage of the local class solution is that you can't directly access local variables in your cleanup code. So it will bloat your code a lot if you need access to them regardless:

void foo() {
    Task* task;
    while (task = nextTask()) {
        task->status = running;
        struct Finally {
            Task* task;
            Finally(Task* task) : task(task) {}
            ~Finally() { task->status = idle; }
        } finalizer(task);
        // ...code that might throw an exception...
    }
}

So my question is: Is there a solution which combines both advantages? So that you a) don't have to write duplicate code and b) can access local variables in the cleanup code, like task in the last example, but without such code bloat.

rlerallut
  • 7,545
  • 5
  • 23
  • 21
Sascha
  • 1,410
  • 2
  • 10
  • 10
  • That is Ugly. You should create a RunObject or something!!! – Martin York Nov 20 '08 at 17:26
  • +1 for asking this because it's so common to see people who are not familiar with `RAII` thinking that `finally` is good... – Piotr Dobrogost Jun 13 '10 at 20:50
  • "you don't have to write the cleanup code 2 times". Why would you ever write code twice, for any reason? That's what subroutines are for =P Neutral on the question of RAII vs finally, but writing code twice is a red herring: you could create a cleanup routine, pass task to it, and call it in two places. No code duplication needed. In your simple example, that logically is part of task's destructor. But if there IS some reason you want cleanup in foo, then create a local cleanup routine there. – ToolmakerSteve Nov 21 '13 at 02:54

5 Answers5

16

Instead of defining struct Finally, you could extract your cleanup code in a function of the class Task and use Loki's ScopeGuard.

ScopeGuard guard = MakeGuard(&Task::cleanup, task);

See also this DrDobb's article and this other article for more about ScopeGuards.

Sébastien RoccaSerra
  • 16,731
  • 8
  • 50
  • 54
  • One detail to be wary of is that if the cleanup code itself throws, using ScopeGuard will silently discard the thrown exception (which certainly is reasonable given C++'s exception-during-destruction handling) - the op's first example will terminate and the second will throw the new exception from the cleanup code instead of the original exception, if any – Jacek Sieka Jul 18 '12 at 12:50
9

I don't think there is a cleaner way to achieve what you're trying to do, but I think the main problem with the 'finally approach' in your example is an improper separation of concerns.

E.g. the function foo() is responsible for the Task object's consistency, this is rarely a good idea, the methods of Task themselves should be responsible for setting the status to something sensible.

I do realise sometimes there is a real need for finally, and your code is obviously just a simple example to show a point, but those cases are rare. And a bit more contrived code in rare cases is acceptable to me.

What I'm trying to say is, you should rarely have a need for finally constructs, and for the few cases where you do, I'd say don't waste time on constructing some nicer way. It will only encourage you to use finally more than you really should...

Pieter
  • 17,435
  • 8
  • 50
  • 89
6

That is such an ugly way of doing it: (are you coming from Java?)

Please Read this article:
Does C++ support 'finally' blocks? (And what's this 'RAII' I keep hearing about?)

It explains why finally is such an ugly concept and why RIAA is much more elegant.

Community
  • 1
  • 1
Martin York
  • 257,169
  • 86
  • 333
  • 562
2

I normally use something more like this:

class Runner {
private:
  Task & task;
  State oldstate;
public:
  Runner (Task &t, State newstate) : task(t), oldstate(t.status); 
  {
    task.status = newstate;
  };

  ~Runner() 
  {
    task.status = oldstate;
  };
};

void foo() 
{
  Task* task;
  while (task = nextTask())
  {
    Runner r(*task, running);
            // ...code that might throw an exception...
  }
}
Roddy
  • 66,617
  • 42
  • 165
  • 277
1

As others have said, the "solution" is better separation of concerns. In your case, why can't the task variable take care of cleaning up after itself? If any cleanup needs to be done on it, then it shouldn't be a pointer, but a RAII object.

void foo() {
//    Task* task;
ScopedTask task; // Some type which internally stores a Task*, but also contains a destructor for RAII cleanup
    while (task = nextTask()) {
        task->status = running;
        // ...code that might throw an exception...
    }
}

Smart pointers may be what you need in this case (boost::shared_ptr will delete the pointer by default, but you can specify custom deleter functions instead, which can perform arbitrary cleanup takss instead. For RAII on pointers, that's usually what you'll want.

The problem isn't the lack of a finally keyword, it's that you use raw pointers, which can't implement RAII.

But usually, every type should know how to clean up after itself. Not after every object that were in scope when the exception was thrown (which is what finally does, and what you were trying to do), just after itself. And if every object does that, then you don't need the big catch-all "clean up after every object in scope" function at all.

jalf
  • 243,077
  • 51
  • 345
  • 550
  • deleted my comment, as my socket example was meh :) i think i agree there are few cases where finally can be used. but it would be nice anyway to have/simulate it :) – Johannes Schaub - litb Nov 20 '08 at 16:23