Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
#include <boost/thread/mutex.hpp>

class Singleton
{
public:
  static Singleton& GetInstance()
  {
    boost::mutex::scoped_lock lock(m_mutex);
    static Singleton instance;
    return instance;
  }

private:
  static boost::mutex m_mutex;

  Singleton() {}
  ~Singleton() {}

  Singleton(const Singleton&);
  const Singleton& operator=(const Singleton&);
};

I have created the above class to simulate a thread-safe Singleton.

Question> Do I miss anything?

share|improve this question
5  
It is, however, a well-posed question. Many people on SO will chide you vigorously for implementing a Singleton in the first place. But I believe that questions should be up- or down-voted based mainly on how well the question was asked, not on whether it is the Right or Wrong Thing. Therefore +1 –  John Dibling Nov 30 '11 at 21:03
1  
Looks good to me :) –  Victor Parmar Nov 30 '11 at 21:05
    
Maybe I'm missing something. Why not just declare your singleton where the mutex is declared? Then you wouldn't even need the mutex because you don't have a race for the constructor. –  Karl Bielefeldt Nov 30 '11 at 21:15
    
Agree with Karl, the mutex is pointless, I don't believe there will be a race in the constructor of the static instance in the function... –  Nim Nov 30 '11 at 22:00
2  
@KarlBielefeldt: The point of putting the singelton instance as a static member of a function is so the singelton is lazily created. If you put it with the mutex (at class scope) it will always be created. If the cost of creating the singelton is high you want lazy initialization. By putting the mutex in class scope it is guranteed to be created before main (before any threading starts and thus not substitutable to multi-threaded problems. –  Loki Astari Dec 1 '11 at 0:05
show 3 more comments

migrated from stackoverflow.com Nov 30 '11 at 21:05

This question came from our site for professional and enthusiast programmers.

2 Answers

As usual, when dealing with singletons, consider that you probably shouldn't be making a singleton at all.

Read this, for example. A singleton is almost certainly not what you actually need, even if it is implemented correctly and in a thread-safe manner.

A plain old global might be a better option, or perhaps you should simply pass a reference around to those who need it.

share|improve this answer
    
I didn't realize you had a form of the URL that's easily memorized. :) Yay. –  GManNickG Nov 30 '11 at 22:46
1  
-1: Does not answer the question. He is not asking weather a singelton is good bad ideas. You could just as easily have posted this as a comment. –  Loki Astari Dec 1 '11 at 0:07
4  
@Loki: I looked in the FAQ for this site, and didn't see anything that discouraged answers like this. It says that if you are looking for "specific feedback about &lt;snip&gt; Best practices and design pattern usage in your code", this is the right side. And that "any aspect of the code posted is fair game for feedback and criticism." If I'm conducting a code review, the first thing I try to find out is "is this the right solution to the problem". I considered posting it as a comment, but to be honest, I think it's valid as an answer. –  jalf Dec 1 '11 at 8:04
    
@GMan: yeah, I got tired of googling it... ;) –  jalf Dec 1 '11 at 8:04
    
-1, this kind of answer should be discouraged. It doesn't address the question at all. –  l46kok Jan 28 '13 at 7:30
show 1 more comment

Static m_mutex member isn't proper singleton itself. You can't use this singleton at static variables initialization time, because m_mutex isn't created lazily and may be not initialized.

Also, it's wrong to put actual logic to Singleton class, it should be template class.

#ifndef SINGLETON_H_689E0B11_E731_4555_AFE6_88598B582F1D
#define SINGLETON_H_689E0B11_E731_4555_AFE6_88598B582F1D

#include <cassert>
#include <boost/thread/mutex.hpp>

template<class T>
class Singleton
{
public:
  static T& GetInstance()
  {
    assert(!is_destructed);
    (void)is_destructed; // prevent removing is_destructed in Release configuration

    boost::mutex::scoped_lock lock(GetMutex());
    static T instance;
    return instance;
  }

private:
  static bool is_destructed;

  static boost::mutex& GetMutex()
  {
    static boost::mutex mutex;
    return mutex;
  }

  Singleton() {}
  ~Singleton() { is_destructed = true; }
};

// force creating mutex before main() is called
template<class T>
bool Singleton<T>::is_destructed = (Singleton<T>::GetMutex(), false);

#endif // SINGLETON_H_689E0B11_E731_4555_AFE6_88598B582F1D

As it have private destructor, it's noncopyable - no need to declare copy ctor and copy assignment operator.

share|improve this answer
    
@Abyx, I have problems to understand the line bool Singleton::is_destructed = (Singleton::GetMutex(), false);. Which syntax you used to build this assignment? –  q0987 Dec 1 '11 at 16:14
    
@q0987, it's C++ syntax. –  Abyx Dec 1 '11 at 18:08
    
@Loki Astari, I didn't wrote that this code should be placed into one header file. And I didn't wrote anonymous namespace there. –  Abyx Dec 1 '11 at 18:48
    
@Loki Astari, I don't get your first comment. There is no problems with mutex initialization, it will be either initialized with is_destructed initialization, either before it, on GetInstance call. –  Abyx Dec 2 '11 at 5:47
    
@Loki Astari, I updated the code, now it's complete header file (without .cpp file required). –  Abyx Dec 2 '11 at 5:56
show 1 more comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.