Disallowing creation of the temporary objects

Question

While debugging crash in a multithreaded application I finally located the problem in this statement:

CSingleLock(&m_criticalSection, TRUE);

Notice that it is creating an unnamed object of CSingleLock class and hence the critical section object gets unlocked immediately after this statement. This is obviously not what the coder wanted. This error was caused by a simple typing mistake. My question is, is there someway I can prevent the temporary object of a class being created at the compile time itself i.e. the above type of code should generate a compiler error. In general, I think whenever a class tries to do some sort of resource acquisition then the temporary object of that class should not be allowed. Is there any way to enforce it?

Answer 1

Compiler shouldn't disallow temporary object creation, IMHO.

Specially cases like shrinking a vector you really need temporary object to be created.

std::vector<T>(v).swap(v);

Though it is bit difficult but still code review and unit testing should catch these issues.

Otherwise, here is one poor man's solution:

CSingleLock aLock(&m_criticalSection); //Don't use the second parameter whose default is FALSE

aLock.Lock();  //an explicit lock should take care of your problem

Answer 2

What about the following? Slightly abuses the preprocessor, but it's clever enough that I think it should be included:

class CSingleLock
{
    ...
};
#define CSingleLock class CSingleLock

Now forgetting to name the temporary results in an error, because while the following is valid C++:

class CSingleLock lock(&m_criticalSection, true); // Compiles just fine!

The same code, but omitting the name, is not:

class CSingleLock(&m_criticalSection, true); // <-- ERROR!

Answer 3

I don't think so.

While it's not a sensible thing to do - as you've found out with your bug - there's nothing "illegal" about the statement. The compiler has no way of knowing whether the return value from the method is "vital" or not.

Answer 4

First, Earwicker makes some good points -- you can't prevent every accidental misuse of this construct.

But for your specific case, this can in fact be avoided. That's because C++ does make one (strange) distinction regarding temporary objects: Free functions cannot take non-const references to temporary objects. So, in order to avoid locks that blip into and out of existence, just move the locking code out of the CSingleLock constructor and into a free function (which you can make a friend to avoid exposing internals as methods):

class CSingleLock {
    friend void Lock(CSingleLock& lock) {
        // Perform the actual locking here.
    }
};

Unlocking is still performed in the destructor.

To use:

CSingleLock myLock(&m_criticalSection, TRUE);
Lock(myLock);

Yes, it's slightly more unwieldy to write. But now, the compiler will complain if you try:

Lock(CSingleLock(&m_criticalSection, TRUE));   // Error! Caught at compile time.

Because the non-const ref parameter of Lock() cannot bind to a temporary.

Perhaps surprisingly, class methods can operate on temporaries -- that's why Lock() needs to be a free function. If you drop the friend specifier and the function parameter in the top snippet to make Lock() a method, then the compiler will happily allow you to write:

CSingleLock(&m_criticalSection, TRUE).Lock();  // Yikes!

MS COMPILER NOTE: MSVC++ versions up to Visual Studio .NET 2003 incorrectly allowed functions to bind to non-const references in versions prior to VC++ 2005. This behaviour has been fixed in VC++ 2005 and above.

Answer 5

I see that in 5 years nobody has come up with the most simple solution:

#define LOCK(x) CSingleLock lock(&x, TRUE);
...
void f() {
   LOCK(m_criticalSection);

And now only use this macro for creating locks. No chance to create temporaries any more! This has the added benefit that the macro can be easily augmented to perform any kind of checking in debug builds, for example detecting inappropriate recursive locking, recording file and line of the lock, and much more.

Answer 6

Edit: As j_random_hacker notes, it is possible to force the user to declare a named object in order to take out a lock.

However, even if creation of temporaries was somehow banned for your class, then the user could make a similar mistake:

// take out a lock:
if (m_multiThreaded)
{
    CSingleLock c(&m_criticalSection, TRUE);
}

// do other stuff, assuming lock is held

Ultimately, the user has to understand the impact of a line of code that they write. In this case, they have to know that they're creating an object and they have to know how long it lasts.

Another likely mistake:

 CSingleLock *c = new CSingleLock(&m_criticalSection, TRUE);

 // do other stuff, don't call delete on c...

Which would lead you to ask "Is there any way I can stop the user of my class from allocating it on the heap"? To which the answer would be the same.

In C++0x there will be another way to do all this, by using lambdas. Define a function:

template <class TLock, class TLockedOperation>
void WithLock(TLock *lock, const TLockedOperation &op)
{
    CSingleLock c(lock, TRUE);
    op();
}

That function captures the correct usage of CSingleLock. Now let users do this:

WithLock(&m_criticalSection, 
[&] {
        // do stuff, lock is held in this context.
    });

This is much harder for the user to screw up. The syntax looks weird at first, but [&] followed by a code block means "Define a function that takes no args, and if I refer to anything by name and it is the name of something outside (e.g. a local variable in the containing function) let me access it by non-const reference, so I can modify it.)