11

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?

StackedCrooked
  • 34,653
  • 44
  • 154
  • 278
  • 4
    "This patch works fine" - up to the point where you start using namespaces. – Steve Jessop Mar 01 '11 at 18:58
  • 1
    @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). – Jeremiah Willcock Mar 01 '11 at 19:01
  • 1
    @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). – Steve Jessop Mar 01 '11 at 19:02
  • @Steve: Yes, that's true, and the macro will break having anything named `ScopedLock` in another namespace. – Jeremiah Willcock Mar 01 '11 at 19:07
  • See also http://stackoverflow.com/questions/914861/disallowing-creation-of-the-temporary-objects which may or may not be a duplicate. – Mark B Mar 01 '11 at 19:44

2 Answers2

8

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.)

Greg Hewgill
  • 951,095
  • 183
  • 1,149
  • 1,285
  • Thanks. I have experimented with Cppcheck a few times and found it useful. Perhaps I should work on integrating it with our build system. – StackedCrooked Mar 01 '11 at 19:43
  • @StackedCrooked: The Cppcheck development is proceeding at a pretty rapid pace. If you haven't tried it recently, definitely try it again. – Greg Hewgill Mar 01 '11 at 19:54
3

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.

Steve Jessop
  • 273,490
  • 39
  • 460
  • 699
  • 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. – Mahmoud Al-Qudsi Mar 01 '11 at 19:48
  • @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). – Steve Jessop Mar 01 '11 at 21:03