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?

Religion

This is similar to GCC's attribute ((warn_unused_result)) (ohse.de/uwe/articles/gcc-attributes.html#func-used), except it appears that can not be used on constructors. – Separator

classical problem with wrappers for sync objects described in Robbins' book. Prohibit acquiring access from constructor; client would be forced to use Lock-methods explicitly – Staal

@Andrey: What book is that? TIA. – Frock

Answer 1

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

Uncanny

+1, many good points. But although we can't fix them all, we can fix the OP's particular case by moving the locking into a free function that takes a non-const reference to a CSingleLock -- this will refuse to bind to a temporary. – Frock

True, so I've added a mention of it, but I don't think it changes the thrust of what I'm saying. (Nor does my C++0x suggestion either - a dumb user could call WithLock, pass it an empty lambda and then write the rest of their function expecting the lock to stay held). – Uncanny

Sure, try to make something idiot-proof and the universe will produce a better idiot :) – Frock

"When designing something completely foolproof, never underestimate the ingenuity of complete fools" - Douglas Adams. – Uncanny

Answer 2

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.

Frock

I'd really argue that this makes it more error prone because it makes it easier to forget to actually lock the lock. Also, it directly negates the whole idea of RAII. Well, half. The namesake half that is – Privett

@sehe: Because it now requires 2 statements? I see what you mean. Thinking about it, I think the root cause of the problem is C++'s conscious decision to allow temporary objects to be created implicitly without any way of constraining this. (This also creates confusion and removes safety/predictability of type conversions.) – Frock

I haven't worked it out, but I have a hunch that with standard c++0x's Move (constructor) semantics and a static factory method instead of a public constructor this would be fixable with something like Guard g(Guard::For(lock)); i.e. one would make all constructors except the move constructor private. You could get it more succinct using type inference and a freestanding friend factory: auto guard(::monitor(lock)); - someone up for the task ... ? – Privett

@sehe: You may be onto something, but what's to stop someone writing only Guard::For(lock); and incorrectly thinking they have locked lock? (That's the equivalent of writing just CSingleLock myLock(&m_criticalSection, TRUE); in my answer.) – Frock

@sehe: I think it will be hard to guarantee correct usage at compile time, but you could certainly arrange for the destructor of the object returned by Guard::For() to check internal state and cause an explosion if it has not been moved via Guard's move ctor, so you would know at runtime as soon as you hit the incorrect lock. Hmm, I think this approach would also work just with C++03's plain ctors! – Frock

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.

Litman

Answer 4

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

Murex

Hence why RAII should be called RRID (Resource Release Is Destruction). – Chatterer

Well, although unlikely, it's still possible to write the incorrect code "CSingleLock(&m_criticalSection).Lock()". You need to make Lock() a free function. – Frock

Answer 5

No, there is no way of doing this. Doing so would break almost all C++ code which relies heavily on creating nameless temporaries. Your only solution for specific classes is to make their constructors private and then always construct them via some sort of factory. But I think the cure is worse than the disease!

Schizogenesis

-1 sorry. It can be done by moving the locking code to a free function that binds a non-const ref to a CSingleLock -- this is not allowed to bind to a temporary. – Frock

The question I was answering is the one asked by the questioner "My question is, is there someway I can prevent the temporary object of a class being created at the compile time itself ", and my answer is correct as far as that goes. Also, can I observe that voting down answers in a discussion you yourself are are taking part in is not very good practice. – Schizogenesis

Actually you're right about what the question asked. (I interpreted the "underlying" question to be "How do you prevent misuse of temporaries", not "How do you prevent temporaries".) If you can tell me how your suggestion of using a factory reduces the chances of making this type of error I'll happily +1, but on the face of it I'd say it's as easy to forget to use a factory function's return value as it is to construct an immediately-destructed temporary. – Frock

Regarding voting, I try to vote on merit and hope others will do the same. – Frock

Answer 6

You can cause a compiler warning using [[nodiscard]]

class CSingleLock {
 public:
  [[nodiscard]] CSingleLock (std::mutex*, bool) { }
};

If you create a temporary clang will warn you saying:

warning: ignoring temporary created by a constructor declared with 'nodiscard' attribute [-Wunused-value]
  CSingleLock(&m, true);
  ^~~~~~~~~~~~~~~~~~~~~

GCC 9.3 seems to have a problem with [[nodiscard]] on constructors though. It still gives additional warnings if you don't use the result. The problem is fixed in gcc 10+ and it produces a similar (but less-specific) warning.

---

Another possible solution: by define a macro function with the same name as the class, you can trigger a static assertion with a helpful message when someone forgets the variable name. live here

class CSingleLock {
 public:
  CSingleLock (std::mutex*, bool) { }
};

// must come after class definition
#define CSingleLock(...) static_assert(false, \
    "Temporary CSingleLock objects are forbidden, did you forget a variable name?")

The macro won't match when there is a variable name. However, this doesn't help in the case of uniform initialization; you can't catch CSingleLock{&m, true}. PfhorSlayer's answer works with uniform initialization so it is safer to use, at the cost of a more confusing error message. I would still reccomend that solution over mine. Unfortunately all these macro solutions fail when the type is in a namespace.

Epsom

Answer 7

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.

Lydie

Answer 8

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!

Fitting