snip

...

Sorry, I don't have an answer to your specific question,

but why would

...

you need to use a shared_ptr on a singleton anyway? You have no real need to deallocate a singleton instance, so no need for a shared_ptr....perhaps use a reference (See Meyers singletons)

Oh, hold on a sec....is it valid to call get() on an unassigned shared_ptr, which you are doing in the instance function?

James

Back when I first learned about the singleton pattern I used a standard pointer similar to this example:

class Singleton { public: static Singleton* Instance(); protected: Singleton(); Singleton(const Singleton&); Singleton& operator= (const Singleton&); private: static Singleton* pinstance; };

Now I am trying to be a good programmer and keep track of my pointers. So I decided that I should replace the pointer with a shared_ptr.

In regards to the validity of calling get() on an unassigned shared_ptr I thought that if a shared_ptr was declared in the header and no call is given to any constructor that the default constructor would be used. Therefore the shared_ptr would have a value of 0x0.

Stephen

Wasn't sure about the get(), but with regard to the singleton itself, there is no real need to deallocate the singleton instance as the only memory leak is at the termination of the program and the memory is aonly allocated once, so there is no need to reference count the people using the singleton (which would be the purpose of the shared_ptr). I'd just be inclined to either use your original code, or go with the Meyers singleton which returns a reference (no pointers at all). Just take care if multi threaded. James This message (including any attachments) contains confidential and/or proprietary information intended only for the addressee. Any unauthorized disclosure, copying, distribution or reliance on the contents of this information is strictly prohibited and may constitute a violation of law. If you are not the intended recipient, please notify the sender immediately by responding to this e-mail, and delete the message from your system. If you have any questions about this e-mail please notify the sender immediately.

Here is the latest form using what I believe is the Meyer form of the singleton. What I was using earlier was from the GoF book on patterns. They use a pointer and instead of a static variable. I would appreciate any comments on this code. The intent is to provide a single-threaded tracing capability which is logged to a file. ----------- HEADER ----------------- #ifndef LIBREVERSE_TRACE_H #define LIBREVERSE_TRACE_H #include <boost/cstdint.hpp> #include <boost/shared_ptr.hpp> #include <fstream> #include "Reverse.h" namespace libreverse { namespace trace { /* Idea taken from http://www.codeproject.com/debug/xytrace.asp * (28 Jan 2002 - Xiangyang Liu) * * I have modified this so that we don't use varargs and use constant types */ class Trace_State { public: typedef boost::shared_ptr<Trace_State> ptr_t; static Trace_State& Instance(); void set_Trace_File_Prefix ( std::string name ); void set_Trace_Level ( boost::uint32_t level ); void open_Trace_File ( void ); std::string get_ID_String ( void ); void close_Trace_File ( void ); boost::uint32_t get_Trace_Level ( void ) const; void write_Message ( boost::uint32_t level, std::string msg ); private: Trace_State(); virtual ~Trace_State(); std::string m_file_prefix; boost::uint32_t m_trace_level; std::ofstream m_log_stream; }; class Trace { public: static bool write_Trace ( boost::uint32_t level, std::string message ); }; } /* namespace trace */ } /* namespace trace */ #endif /* LIBREVERSE_TRACE_H */ --------- SOURCE ------------- #include "Trace.h" #include <boost/format.hpp> #include <boost/date_time/posix_time/posix_time.hpp> #include <sstream> #ifndef WIN32 #include <unistd.h> #else #include <windows.h> #endif /* WIN32 */ namespace libreverse { namespace trace { Trace_State& Trace_State::Instance() { static Trace_State instance_obj; return instance_obj; } void Trace_State::set_Trace_File_Prefix ( std::string name ) { assert ( ! name.empty() ); // Lock the resource // Close the present file m_file_prefix = name; // Unlock the resource } void Trace_State::set_Trace_Level ( boost::uint32_t level ) { // Lock the resource // Change level m_trace_level = level; // Unlock the resource } void Trace_State::open_Trace_File ( void ) { if ( ! m_log_stream.is_open() ) { // Create file name std::stringstream name; name << boost::format("%s_%s.txt") % m_file_prefix % this->get_ID_String(); m_log_stream.open ( (name.str()).c_str() ); } } std::string Trace_State::get_ID_String ( void ) { // Create id string std::stringstream name; // Get current time boost::posix_time::ptime now = boost::posix_time::second_clock::local_time(); std::tm tm_ref = boost::posix_time::to_tm ( now ); boost::gregorian::date today = now.date(); name << boost::format ( "%s_%02d:%02d:%02d" ) % boost::gregorian::to_iso_extended_string ( today ) % tm_ref.tm_hour % tm_ref.tm_min % tm_ref.tm_sec; return name.str(); } void Trace_State::close_Trace_File ( void ) { if ( m_log_stream.is_open() ) { m_log_stream.close(); } } boost::uint32_t Trace_State::get_Trace_Level ( void ) const { boost::uint32_t level = 0; // Lock the resource // get the level level = m_trace_level; // unlock the resource // return the level return level; } void Trace_State::write_Message ( boost::uint32_t level, std::string msg ) { // Write ID m_log_stream << boost::format("%s_%d: " ) % this->get_ID_String() #ifndef WIN32 % getpid() #else % GetCurrentProcessId() #endif /* WIN32 */ << std::flush; // Write message prefix if ( level == libreverse::api::TraceLevel::TraceWarn ) { m_log_stream << "(WW) "; } else if ( level == libreverse::api::TraceLevel::TraceError ) { m_log_stream << "(EE) "; } else if ( level == libreverse::api::TraceLevel::TraceInfo ) { m_log_stream << "(II) "; } else if ( level == libreverse::api::TraceLevel::TraceDebug ) { m_log_stream << "(DEBUG) "; } else if ( level == libreverse::api::TraceLevel::TraceDetail ) { m_log_stream << "(DETAIL) "; } else if ( level == libreverse::api::TraceLevel::TraceData ) { m_log_stream << "(DATA) "; } else { // We should not be here abort(); } // Write to the file m_log_stream << msg << std::endl << std::flush; // Unlock the resource } Trace_State::Trace_State() : m_file_prefix ( "Trace" ), m_trace_level ( libreverse::api::TraceLevel::TraceNone ) {} Trace_State::~Trace_State() { this->close_Trace_File(); } #ifdef LIBREVERSE_DEBUG bool Trace::write_Trace ( boost::uint32_t level, std::string message ) { // If the level is equal to or greater than the present // level we record out message. if ( ( Trace_State::Instance().get_Trace_Level() != 0 ) && ( level <= Trace_State::Instance().get_Trace_Level() ) ) { Trace_State::Instance().write_Message ( level, message ); } return true; } #else bool Trace::write_Trace ( boost::uint32_t, std::string ) { return true; } #endif } /* namespace trace */ } /* namespace trace */

On Thu, 2007-05-17 at 16:59 +0200, Leon Mlakar wrote:

...

Now I am trying to be a good programmer and keep track of my pointers. So I decided that I should replace the pointer with a shared_ptr.

In regards to the validity of calling get() on an unassigned shared_ptr I thought that if a shared_ptr was declared in the header and no call is given to any constructor that the default constructor would be used. Therefore the shared_ptr would have a value of 0x0.

It would, and you shouldn't get the message. Perhaps the message comes from the copy assignment (which again, it shouldn't) - try

m_instance.reset(new Trace_State());

instead.

You could also simply write if (!m_instance) as shared_ptr declares operator ! just for this purpose.

But I agree, in a singleton pattern you benefit from shared pointer only if the singleton lives shorter than the application and you want to make sure it's not gone while still in use.

Leon

Since its suppose to live the length of the application then its alright that we make it follow the Meyers Singleton without using the boost shared_ptr. Thanks for the help. Stephen