Policy based approach with logger [closed]

Question

I spend some time with redesigning a logger class I did once into a policy based approach after reading an article about policy based design and wanting to try something myself.

Some code:

template <class Filter, class Formatter, class Outputter>
    class LoggerImpl : public LoggerBase {
    public:
        LoggerImpl(const Filter& filter = Filter(), const Formatter& formatter = Formatter(), const Outputter& outputter = Outputter());
        ~LoggerImpl();

        void log(int channel, int loglevel, const char* msg, va_list list) const;
    private:
        const Filter mFilter;
        const Formatter mFormatter;
        const Outputter mOutputter;
    };

template <class Filter, class Formatter, class Outputter>
LoggerImpl<Filter, Formatter, Outputter>::LoggerImpl(const Filter& filter, const Formatter& formatter, const Outputter& outputter) : 
            mFilter(filter), mFormatter(formatter), mOutputter(outputter) {
                debuglib::logdispatch::LoggerMgr.addLogger(this);
        }

typedef LoggerImpl<NoFilter, SimpleFormatter, ConsoleOutputter> ConsoleLogger;
typedef LoggerImpl<ChannelFilter, SimpleFormatter, VSOutputter> SimpleChannelVSLogger;
typedef LoggerImpl<NoFilter, SimpleFormatter, FileOutputter> FileLogger;

ConsoleLogger c;
SimpleChannelVSLogger a(const ChannelFilter(1));
FileLogger f(NoFilter(), SimpleFormatter(), FileOutputter("log.txt"));

// macro for sending log message to all current created loggers
LOG(1, WARN, "Test %d", 1423);

Depending on the logger I need to pass additional information like the logchannel within the SimpleChannelVsLogger or the filename of the logfile in the FileOututter.

I'm passing the parameters to the constructor of LoggerImpl as const reference and copy them into the object stored in the logger class afterwards. There is a need to copy them because the lifetime extension is not transitive through a function argument that occurs when binding the temporary created object to a const reference (more on this here: Does a const reference prolong the life of a temporary?).

So first thing here: If I don't want to use pointers as I am not interested in runtime allocation when using templates, I guess there is no other solution than copying the temporary created objects in the way like above?

The actual problem in the copying stuff now comes with the FileOutputter: An ofstream cannot be copied of course, so how can I copy the FileOutputter object containing the stream? I came up with following solution to overcome this problem:

struct FileOutputter {
            // c_tor
            FileOutputter() {}

            // c_tor
            explicit FileOutputter(const char* fname) {
                mStream = std::make_shared<std::fstream>(fname, std::fstream::out);
            }

            // The copy c_tor will be invoked while creating any logger with FileOutputter
            // FileLogger f(NoFilter(), SimpleFormatter(), FileOutputter("log.txt"));
            // as all incoming paramters from the constructors stack frame are copied into the current instance of the logger
            // as copying a file-stream is not permitted and not good under any means 
            // a shared pointer is used in the copy c_tor 
            // to keep the original stream until no reference is referring to it anymore
            FileOutputter(const FileOutputter& other)  {
                mStream = other.mStream;
            }

            ~FileOutputter() {
            }

            void out(const char* msg) const {
                *mStream << msg;
            }

            std::shared_ptr<std::fstream> mStream;
        };

Somehow I have to feeling that this seems a bit complex for a "simple logger class", however this may be just a "problem" with the policy based design approach in this case.

Any thoughts are welcome

Answer 1

It is correct that you should copy the objects if you are going to store them as members in your class.

Storing references is dangerous as it is possible to pass temporary objects as parameters to your ctor, which will lead to dangling references when the temporaries are destructed.

Passing the parameters as pointers is an alternative, but this aproach is also problematic as it then becomes possible to pass in a nullptr (NULL-value), and you have to check for this.

Another alternative would be to move the values i.e. pass the parameters as r-value references. This will avoid copying, however it will require the client to either pass temporaries or std::move objects when calling the ctor. It would no longer be possible to pass l-value references.

// Define ctor as taking r-value references.
template <class Filter, class Formatter, class Outputter>
LoggerImpl<Filter, Formatter, Outputter>::LoggerImpl(Filter&& filter, Formatter&& formatter, Outputter&& outputter) : 
        mFilter(std::move(filter)), mFormatter(std::move(formatter)), mOutputter(std::move(outputter)) {
            // ...
}

/* ... */

// Calling ctor.
FileLogger f1(NoFilter(), SimpleFormatter(), FileOutputter("log.txt")); // OK, temporaries.
FileOutputter fout("log.txt");
FileLogger f2(NoFilter(), SimpleFormatter(), fout); // Illegal, fout is l-value.
FileLogger f3(NoFilter(), SimpleFormatter(), std::move(fout)); // OK, passing r-value. fout may not be used after this!

If you decide to go with the copy-approach then I recommend to instead pass your parameters by value in the ctor. This will allow the compiler to perform optimizations as copy elision (read: Want Speed? Pass by Value).

template <class Filter, class Formatter, class Outputter>
LoggerImpl<Filter, Formatter, Outputter>::LoggerImpl(Filter filter, Formatter formatter, Outputter outputter) : 
        mFilter(std::move(filter)), mFormatter(std::move(formatter)), mOutputter(std::move(outputter)) {
            // ...
}

Using the above definition: in the best case scenario the compiler will elide the copy and the members will be move constructed (when passing a temporary object).

In the worst case scenario: a copy and a move construction will be performed (when passing an l-value).

Using your version (passing parameters as reference to const), a copy will always be performed as the compiler can not perform optimizations.

For move construction to work, you will have to make sure that the types that are passed as parameters is move constructible (either implicitly or using a declared move ctor). If a type is not move constructible it will be copy constructed.

When it comes to copying the stream in FileOutputter, using std::shared_ptr seems like a good solution, although you should initialize mStream in the initialization list instead of assigning in the ctor body:

explicit FileOutputter(const char* fname)
    : mStream(std::make_shared<std::ofstream>(fname)) {}

// Note: Use std::ofstream for writing (it has the out-flag enabled by default).
//       There is another flag that may be of interest: std::ios::app that forces
//       all output to be appended at the end of the file. Without this, the file
//       will be cleared of all contents when it is opened.

A std::ofstream is non-copyable and passing around a smart pointer (make sure to use std::shared_ptr though) is probably the simplest solution in your case and also in my opinion, contrary to what you say, not overy complex.

Another approach would be to make the stream member static, but then every instance of FileOutputter would use the same std::ofstream object and it would not be possible to use parallel logger objects writing to different files etc.

Alternatively you could move the stream as std::ofstream is non-copyable but movable. This would however require you to make FileOutputter movable and non-copyable (and probably LoggerImpl as well), as using a "moved" object, other than its dtor, can result in UB. Making an object that manages move-only types to itself become move-only may make a lot of sense sometimes though.

std::ofstream out{"log.txt"};
std::ofstream out2{std::move(out)} // OK, std::ofstream is moveable.
out2 << "Writing to stream";       // Ok.
out << "Writing to stream";        // Illegal, out has had its guts ripped out.

Also, in the example provided, you don't need to declare a copy ctor or a dtor for FileOutputter, as they will be implicitly generated by the compiler.

Answer 2

You can have the policy classes contain static functions so ideally you would want FileOutputter to look like:

    template<std::string fileName>
    struct FileOutputter {

        static void out(const char* msg) const {
            std::ofstream out(fileName);
            out << msg;
        }
    };

You would create an instance of LoggerImpl like this

LoggerImpl<NoFilter, SimpleFormatter, FileOutputter<"log.txt"> > FileLogger;

This would mean your LoggerImpl doesn't need to store a copy of the policy classes it just need to call their static functions. Unfortunately this won't work because you can't have strings as template arguments but you can build a table of strings and pass the index of the filename in your string table. So again you would ideally want this to look like:

//This class should be a singleton
class StringManager
{
  std::vector<std::string> m_string;
public:
  void addString(const std::string &str);
  const std::string &getString(int index);
  int getIndexOf(const std::string &str);
};

Then your FileLogger would get an int as a template parameter and it would be the index of the string in the StringManager. This also wouldn't quite work because you need the index available at compile time and StringManager would be initialized at run time. So you would have to build the string table manually and manually write in the index of your string. so your code would look like (after you make StringManager a singleton:

StringManager::instance()->addString("a");
StringManager::instance()->addString("b");
StringManager::instance()->addString("c");
StringManager::instance()->addString("d");
StringManager::instance()->addString("log.txt");
LoggerImpl<NoFilter, SimpleFormatter, FileOutputter<4> > FileLogger;

You would have to make sure that StringManager is fully initialized before the first instance of FileLogger is created. It's a little ugly but using templates with strings is a little ugly. You could also do something like:

template <class FilenameHolder>
struct FileOutputter {

        static void out(const char* msg) const {
            std::ofstream out(FilenameHolder::fileName());
            out << msg;
        }
    };

class MyFileNameHolder
{
  static std::string fileName() {return "log.txt";}
};