Concerning RAII: How to prevent errors caused by accidentally creating a temporary?

Question

A while age a colleague told me he spent lot of time debugging a race condition. The culprit turned out to be something like this:

void foo()
{
    ScopedLock(this->mutex); // Oops, should have been a named object.
                             // Edit: added the "this->" to fix compilation issue.
    // ....
}

In order to prevent situation from happening again he created the following macro after the definition of the ScopedLock class:

#define ScopedLock(...) Error_You_should_create_a_named_object;

This patch works fine.

Does anyone know any other interesting techniques to prevent this problem?

Rockwood

"This patch works fine" - up to the point where you start using namespaces. – Barriebarrientos

@Steve: It will work fine with namespaces as well, as long as you don't have anything named ScopedLock in another namespace. The macro will still work namespace-qualified (since it produces an error anyway). – Frothy

@Jeremiah: the sole purpose of namespaces is to allow you to have something named ScopedLock in another namespace. Or specifically, to write code so that you don't care whether or not someone else likes the look of the name ScopedLock for their class (function, or global variable). – Barriebarrientos

@Steve: Yes, that's true, and the macro will break having anything named ScopedLock in another namespace. – Frothy

See also #915361 which may or may not be a duplicate. – Billibilliard

Answer 1

If you're going to define a macro, I'd probably rather define this one:

#define GET_SCOPED_LOCK(name, mtx) ScopedLock name(mtx)

and stop creating objects other than via the macro.

Then rename ScopedLock to ThisClassNameShouldNotAppearInUserCode if that helps.

Barriebarrientos

You could do with the "name" in the Macro. If it's a scope lock, you won't have two different objects in the same scope, so you can use a "constant" name without any worries regarding duplication. – Paleethnology

@Computer Guru: You could always provide that too for convenience. It seems to me a bit intrusive to introduce a name into scope without the user really seeing what that name is (other than in the documentation). Using a compiler-generated unique name would deal with that, as long as the object provides no functions for the user to call. Also a bit awkward that if you want to lock two mutexes you have to introduce braces (and possibly deal with compiler warnings about shadowing the outer variable). – Barriebarrientos

Answer 2

You should use a static code analyser such as Cppcheck. For the following code:

class a { };

void f() {
    a();
}

cppcheck generates the following output:

$ cppcheck test.cpp
Checking test.cpp...
[test.cpp:4]: (error) instance of "a" object destroyed immediately

There are a wide variety of other common coding errors also detected.

(I'm a fairly new contributor to Cppcheck. I found it a couple of months ago and it's been fantastic working with it.)

Hospitalize

Thanks. I have experimented with Cppcheck a few times and found it useful. Perhaps I should work on integrating it with our build system. – Rockwood

@StackedCrooked: The Cppcheck development is proceeding at a pretty rapid pace. If you haven't tried it recently, definitely try it again. – Hospitalize