Registrar Policy Approach

Question

I spend some time redesigning the logger class that I did once in a political approach after reading an article on policy-based development and wanting to try something myself.

Some codes:

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, such as the logchannel inside SimpleChannelVsLogger or the name of the log file in FileOututter.

I pass the parameters to the LoggerImpl constructor as a reference to the constant and then copy them to the object stored in the logger class. It is necessary to copy them, because the extension of the lifespan is not transitive through the function argument, which occurs when the temporary created object is linked with a link to const (more about this here: Does the link const extend the service life? ).

So the first: here, if I do not want to use pointers, since I am not interested in distributing the runtime when using templates, I think there is no other solution than copying temporary created objects in the way described above?

The problem with copying files is now related to FileOutputter: Of course, the stream cannot be copied, so how can I copy the FileOutputter object containing the stream? To solve this problem, I came to the following solution:

 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 feel that this seems a bit complicated for a “simple magazine class”, however in this case it may just be a “problem” with a policy-based approach.

Any thoughts are welcome.

Answer 1

It is true that you must copy objects if you intend to store them as members in your class.

Saving links is dangerous because you can pass temporary objects as parameters to your ctor, which will lead to broken links when temporary files are destroyed.

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

Another alternative would be to move the values, that is, pass parameters as references to the r-value. This will avoid copying, however, the client will need to either pass temporary or std::move objects when ctor is called. It would be impossible to pass references to the l-value.

 // 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 a copying approach, I recommend instead passing your parameters by value to ctor. This will allow the compiler to perform optimization 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 definition above: at best, the compiler will return a copy, and the members will be moved along the path (when passing a temporary object).

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

Using your version (passing parameters as a reference to const), a copy will always be executed, since the compiler cannot perform optimizations.

To create a construction for work , you need to make sure that types that are passed as parameters are moved constructively (either implicitly or using the declared ctor move). If the type does not move constructively, it will be copied.

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

 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 not copied and runs around a smart pointer (be sure to use std::shared_ptr though), is probably the easiest solution in your case, and also, in my opinion, contrary to what you say, not too complicated.

Another approach would be to make the stream element static, but then each FileOutputter instance will use the same std::ofstream , and it would be impossible to use parallel logger objects to write to different files, etc.

Alternatively, you can move the stream, since std::ofstream not copied, but moved. However, this would require making FileOutputter movable and not copied (and probably LoggerImpl ), since using a "moved" object other than its dtor can lead to UB. Creating an object that controls only the types of movement becomes only a movement, but sometimes it can make a lot of sense.

 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. 

In addition, in the above example, you do not need to declare a copy of ctor or dtor for FileOutputter , since they will be implicitly generated by the compiler.

Answer 2

You may have policy classes containing static functions, so you would like FileOutputter to look like this:

  template<std::string fileName> struct FileOutputter { static void out(const char* msg) const { std::ofstream out(fileName); out << msg; } }; 

You would create a LoggerImpl instance like this

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

This would mean that your LoggerImpl does not need to store a copy of the policy classes, which just need to be called their static functions. Unfortunately, this will not work because you cannot have strings as template arguments, but you can build a string table and pass the index of the file name in your string table. So, you would ideally want it to look like this:

 //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 will get an int as a template parameter, and that will be the row index in StringManager. This will not work either, because you need an index available at compile time, and the StringManager will be initialized at runtime. Thus, you will need to build the row table manually and manually write to the index of your row. so your code will look (after you create a StringManager 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 will need to make sure that StringManager is fully initialized before the first instance of FileLogger is created. This is a little ugly, but using patterns with strings is a little ugly. You can 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";} };