views:

199

answers:

6

Hello,

I'm trying to make a wrapper for Ogre (an open source 3D engine) 's standard logging class. I want it to have the same syntax as std::cerr, and also output to cerr when running on Linux. Here's what I have:

#ifndef _LOGGER_H_
#define _LOGGER_H_

#ifndef _XSTRING_
    #include <xstring>
#endif

#ifndef __LogManager_H__
    #include "OgreLogManager.h"
#endif

class Logger
{

public:

    static Logger* m_Instance;
    static Logger* getInstance() { return m_Instance; }
    static const Logger& getInstanceConst() { return *m_Instance; }

    Logger& operator << (const std::string& a_Message)
    {
        m_Log.append(a_Message);
        _CheckNewLine();
        return *m_Instance;
    }

    Logger& operator << (const char* a_Message)
    {
    m_Log += a_Message;
    _CheckNewLine();
    return *m_Instance;
}

private:

    std::string m_Log;

    Logger() 
    {
        m_Log = "";
    }

    void _CheckNewLine()
    {
        if (m_Log.at(m_Log.size() - 1) == '\n')
        {   
            Ogre::LogManager::getSingleton().logMessage(m_Log);

#if OGRE_PLATFORM != PLATFORM_WIN32 && OGRE_PLATFORM != OGRE_PLATFORM_WIN32

            std::cerr << m_Log;

#endif

            m_Log.clear();
        }
    }
};

#endif

Now, this works fine, and this singleton is instanced in a .cpp:

#include "logger.h"

Logger* Logger::m_Instance = new Logger();

The problem comes when I want to use the singleton in multiple headers. I instantiate it in game3d.h, which is included by pretty much all the headers like this:

Logger awesomelogger = Logger::getInstance();

Unfortunately, this gives multiple errors about headers trying to redeclare awesomelogger.

I want to make it a const, which would make that go away, but that introduces new errors. This is what I tried:

friend Logger& operator << (const Logger& a_Logger, const std::string& a_Message)
{
    a_Logger.m_Log.append(a_Message);
    a_Logger._CheckNewLine();
    return *m_Instance;
}

My question is: how can I make an instance of this class constant OR how can I rewrite this class but still be able to do awesomelogger << "output" << s_Stuff << "\n";

+1  A: 

Not to do with your question, but ote that names such as _LOGGER_H_ and __LogManager_H__ are reserverd in C++ - you are not allowed to use them in your own code. If you don't understand the rules regarding underscores at the start of names (or double underscores anywhere), then don't use them.

Now, regarding your question, a logger obviously isn't const. The classic way to provide access to a singleton is some variation on this:

static Logger* getInstance() { 
    static Logger logger;
    return & logger; 
}
anon
Neil, I'm curious what your "standard" way of doing header-guards is?
GMan
Neil is truing to say your macros _LOGGER_H_ is reserved because of the leading underscore. Read this for more details: http://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier/228797#228797
Martin York
Personally I use INC_library/exename_file_H, so something like INC_MYLIB_LOGGER_H.
anon
@Neil: Why return a pointer. Return a reference. It makes it easier to use in a streaming context.
Martin York
@Martin The user wanted a pointer, it seems.
anon
@Neil: No he wanted a variable. His first (incorrect) attempt used a pointer (because he is not as good as you :-).
Martin York
A: 

make a const pointer to a const object.

static const Logger * const m_Instance;

instead of

static Logger* m_Instance;

You'll need a lot of respective corrections in the class too.

modosansreves
+1  A: 

Your get_instance method returns Logger* not Logger.

stribika
A: 

Generally speaking too, your instance of Logger "m_Instance" should be private. And your function "GetLogger()" should check if "m_Instance" has not been instantiated (if not instantiate it) then return m_Instance.

Sam
A: 

The classic singelton pattern looks like this:

#ifndef  THORS_ANVIL_MY_LOGGER_H
#define  THORS_ANVIL_MY_LOGGER_H

class MyLogger
{
  private:
   // Private Constructor
    MyLogger();
   // Stop the compiler generating methods of copy the object
    MyLogger(MyLogger const& copy);           // Not Implemented.
    MyLogger& operator=(MyLogger const& copy); // Not Implemented

  public:
    static MyLogger& getInstance()
    {
        // The only instance
        // Guaranteed to be lazy initialized
        // Guaranteed that it will be destroyed correctly
        static MyLogger instance;
        return instance;
    }
    template<typename T>
    MyLogger& operator<<(T const& data)
    {
          // LOG
          return *this;
    }
};

// continued below.

Now we have the basic pattern out the way. You need what looks like a global variable to access it. The simplest way is to cheat and not have a global, but use a local reference.

So in the header file along with your Logger class definition add the following.

namespace
{
    MyLogger&   logger = MyLogger::getInstance();
}
#endif

This declares a file local variable in every compilation unit that includes the header. That has been initialized to refer to instance of the logger that is a singelton. Because you need to include the file before you can use it will always be declared before any usage and thus guranteed to be initialized in the correct order.

In main file:

#include "MyLogger.h"
int main()
{
    logger << "Plop";
}
Martin York
A: 

It sounds from this:

The problem comes when I want to use the singleton in multiple headers. I instantiate it in game3d.h, which is included by pretty much all the headers like this:

 Logger awesomelogger = Logger::getInstance();

That you are creating a new instance of awesomelogger in every module that includes game3d.h and since this declaration has external linkage, these names will collide at link time.

See Linkage in Names with File Scope for an explanation of C++ linkage rules.

A better solution is to dispense with creating the awesomelogger variable and just call Logger::getInstance() directly wherever you need it.

Jeff Leonard