11

Consider the following code :

void ListenerImpl::attach(boost::shared_ptr<ISubscriber> subscriber)
{
    boost::unique_lock<boost::mutex>(mtx);
    subscribers.push_back(subscriber);
}

void ListenerImpl::notify(MsgPtr msg)
{
    boost::unique_lock<boost::mutex>(mtx);

    //notify all subscribers
    BOOST_FOREACH(boost::shared_ptr<ISubscriber> subscriber, subscribers){
        subscriber->update(msg);
    }

}

(This is an implementation of an observer pattern as described in GoF.) The user intervention here was to protect the attach() and the notify() from simultaneous running, hence the boost::unique_lock. The goal was to protect the subscribers container.

But it is indeed extremely hard to notice that the locks are in fact just temporaries (take a closer look, there are no names assigned for them). So the lock on the mutex will be released immediately, when the temporary is destructed, i.e. the code is not thread safe. I would expect in situations like this a compiler warning. Something like "Unused temporary".

Even worse, cppcheck wouldn't recognize this mistake either. (cppcheck: a c/c++ code analysis tool http://sourceforge.net/apps/mediawiki/cppcheck/index.php?title=Main_Page)

Gcc issues warnings on unused variables. The temporary here is an unused variable, and definitely the result of the programmer's inattention. So, why there are no warnings in cases like this? Maybe it is too complicated to discover such situations?

luke
  • 36,103
  • 8
  • 58
  • 81
Gabor Marton
  • 2,039
  • 2
  • 22
  • 33
  • 1
    I did a similar mistake, check this question too: http://stackoverflow.com/questions/914861/disallowing-creation-of-the-temporary-objects – Naveen Jun 29 '11 at 09:26
  • Did you try -Wall -Wextra as command line arguments to the compiler? I'm not to familiar with C++ though.. – DipSwitch Jun 29 '11 at 09:26
  • Is it guaranteed that an unamed variable (what is referred as *temporary*) is immediately destroyed ? I would have expected it to live until the end of the scope it was defined. – ereOn Jun 29 '11 at 09:40
  • 1
    @ereOn: it lives until the end of the statement :/ – Matthieu M. Jun 29 '11 at 09:53

3 Answers3

8

Compiler doesn't issue a warning, because it's quite possible that you might be updating some static-member / global variable inside the constructor (which is valid and meaningful). e.g.:

struct A
{
  static int count;
  A () { count ++; }
};

Now when you simply invoke a temporary:

A();

In case if such update is not happening, compiler will not dig into the constructor of A and check if something useful is happening. It always assumes to be a valid scenario. There are many such cases can be pointed out related to temporaries.

iammilind
  • 68,093
  • 33
  • 169
  • 336
  • 8
    And indeed, something *is* happening in the case presented by the question: the mutex is locked and released, triggering a cache sync across threads... how can the compiler know that wasn't all that was wanted? – Tony Delroy Jun 29 '11 at 09:44
  • 1
    The compiler should issue warnings every time the programmer does something potentially dangerous. For example, it does when you do something like "if (i = 0)", where you probably wanted to write "if (i == 0)". It is possible that you did this intentionally (e.g. "if (result = doSomething())", but since it's usually not what you want, there is a warning for that. Shouldn't this be the case of temporary variables, too? – petersohn Jun 29 '11 at 10:00
0

Note that your proposed warning will also be issued for every it++;, which is found in many for loops.

iammilind has already mentioned that sometimes it is by intention to create and immediatly destroy the temp: when there are side-effects.

And in template meta programming, one might create and destroy a temporary, just in case the user supplies a class with side-effects. When a simple class, without side-effects, is used to instantiate the template, warnings will appear deep down in the template code.

Therefor, your proposed warning will have many false positives. It will be hard to find the genuine warnings among the spurious ones.

So I expect that compiler vendors have decided that their time is better spent elsewhere.

Sjoerd
  • 6,837
  • 31
  • 44
0

hmm.. I am not sure but can't this be protected against with normal c++ also?

class Mutex;
class Lock {
    Lock(Mutex *mutex);
};

int main() {
    Lock /* lock */ (&mtx);
    return 0;
}

I get this compiler warning when compiling with DJGPP:

C:\df>gxx -c a.cpp
a.cpp: In function 'int main()':
a.cpp:8:30: error: 'mtx' declared as reference but not initialized

It compiles fine if I uncomment "lock" and add a mutex variable.

So if your "mtx" variable is a pointer. What happens if you change it and pass it as "&mtx" instead.

Daniel Marjamäki
  • 1