Writing testable code - a basic_istream factory within lambda functions and unique_ptr

Question

Introduction

This is a request for an assessment of my approach, in the context of modern C++ features that I'm not confident with yet, like lambda functions and basic_istream. I have a short list of specific questions at the end, all of which are related to the same issue.

Background

I am writing a config file parser in C++14 using Boost::program_options (1.69), in particular the boost::program_options::parse_config_file() function. This function (defined in boost/program_options/parsers.hpp) has two overrides.

The first reads the config from a file on disk:

basic_parsed_options<charT>
parse_config_file(const char* filename, const options_description&,
                  bool allow_unregistered = false);

The second reads the config from a std::basic_istream<charT> reference:

basic_parsed_options<charT>
parse_config_file(std::basic_istream<charT>&, const options_description&,
                  bool allow_unregistered = false);

I am writing unit tests (using Google Test) so that I can pass the contents of the configuration file to the function, rather than have the test read from an actual file, as it would in production. This is to avoid having to create temporary config files, avoid conflicts when running tests in parallel, etc. Perhaps this approach could be better, but it doesn't seem wrong to have the ability to provide config from a string rather than a file.

I will also add that I cannot simply pass the istream to the process_config function because, in the full implementation, the filename is actually determined within the function itself. I.e. the command-line is parsed first, and the config filename is taken from this. Therefore the filename is not known before the function is executed.

Implementation

The function under test originally looked like this:

struct AppConfig {
    bool myoption1 {false};
};

void process_config(int argc, char * argv[], AppConfig & config) {
    // ...
    po::parse_config_file("config.cfg", config_file_options);
    // ...
}

The problem is that this interface doesn't provide a way to inject config file contents, so I thought about passing in a callback that process_config() can call in order to return a basic_istream<charT> that it can pass to po::parse_config_file(). That way, the tests can pass in a callback that simply provides a std::istringstream (a sub-class of the basic_istream template) with the config file contents preloaded, and the production client can pass in a simple callback that simply opens the specified file on disk and returns the corresponding istream.

To that end I created this:

template <typename funcF>
void process_config(int argc, char * argv[], AppConfig & config, funcF get_istream) {

    namespace po = boost::program_options;

    po::options_description config_file_options;
    config_file_options.add_options() /* ... */;

    auto istr_up = get_istream("config.cfg");
    auto & istr = *istr_up.get();
    po::parse_config_file(istr, config_file_options);
}

Then in production code, I can use this to return the ifstream from the config file:

auto make_config_istream(const std::string & s) {
    return std::unique_ptr<std::ifstream>(s.c_str());
}

And in the test code I can use this to return the config file contents from a string defined in the test:

std::unique_ptr<std::basic_istream<char>> make_istream(const std::string & s) {
    return std::make_unique<std::istringstream>(s);
}

process_config(0, nullptr, config,
               [](const std::string &){ return make_istream("foo=1\nbar=2");} );

This seems to compile and execute as I'd expect. However it seems a bit clunky because of the ignored const std::string & that has to be present for the lambda signature to match the process_config template, so I attempted to add this additional helper function to hide it away:

auto make_config(const std::string & config) {
    // the std::string parameter is ignored:
    return [config](const std::string &) { return make_istream(config); };
}

And then change the test code to:

process_config(0, nullptr, config, make_config("foo=1\nbar=2"));

This syntax will meet my requirements.

Note that I'm passing back the istream objects within unique pointers. I'm not 100% sure that is the right way to do it, but because the default constructor for istream is disabled, I can't pass them around by value, so it seems I have to allocate them on the heap and pass them around by pointer.

Questions

Thank you for reading this far. My questions are:

1. Is it appropriate / best-practice to return the newly constructed istream on the heap, via the use of a unique_ptr? Is there a better way to do this?
2. Is the approach of dereferencing the unique_ptr to gain access to a reference to the istream correct using auto & istr = *istr_up.get()? This smells bad to me.
3. Is using a templated process_config function the best way to handle the types involved in the get_istream parameter? Is there a better way to do it with auto?
4. Lastly, is there a better approach to this entire issue?

Answer 1

Is it appropriate / best-practice to return the newly constructed istream on the heap, via the use of a unique_ptr? Is there a better way to do this?

basic_istream is movable, you don't need unique_ptr:

auto make_config_istream(const std::string & s) {
    return std::ifstream(s);
}

auto make_istream(const std::string & s) {
    return std::istringstream(s);
}

template <typename funcF>
void process_config(int argc, char * argv[], AppConfig & config, funcF get_istream) {

    namespace po = boost::program_options;

    po::options_description config_file_options;
    config_file_options.add_options() /* ... */;

    auto istr = get_istream("config.cfg");
    po::parse_config_file(istr, config_file_options);
}

The rest can stay as is.

Is the approach of dereferencing the unique_ptr to gain access to a reference to the istream correct using auto & istr = *istr_up.get()? This smells bad to me.

It is correct, but unnecessary complicated because auto & istr = *istr_up; does the same. You can also use istr_up as if it was a raw pointer (i.e. with -> and *).

Is using a templated process_config function the best way to handle the types involved in the get_istream parameter? Is there a better way to do it with auto?

Yes that is the proper way to do it, but requires parse_config to be moved into a header shared by all translation units using it, or otherwise extra care needs to be taken to correctly explicitly instantiate where necessary.

---

The usage of process_config seems too complicated to me, why not:

process_config(0, nullptr, config,
    [](auto){return std::istringstream("foo=1\nbar=2");})

and

process_config(0, nullptr, config,
    [](auto s){return std::ifstream(s);})