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.