Code review comment for lp:~josharenson/mir/command_line_config

23 +#ifndef COMMAND_LINE_SERVER_CONFIGURATION
24 +#define COMMAND_LINE_SERVER_CONFIGURATION

Missing MIR_TEST_FRAMEWORK_ prefix

~~~~

26 +#include "mir/options/default_configuration.h"

We only need a declaration: namespace mir { namespace options { class mo::Configuration; } }

But we do need

#include <memory>

~~~~

28 +namespace mo=mir::options;

Not in a header.

~~~~

30 +namespace mir_test_framework
31 +{
32 + auto configuration_from_commandline()
33 + -> std::shared_ptr<mo::DefaultConfiguration>;

The return type should be mir::options::Configuration;

~~~~

36 +int main(int argc, char** argv);

not wanted.

~~~~

74 +#include "mir/default_configuration.h"
75 +#include "mir/options/default_configuration.h"
76 +#include "mir/default_server_configuration.h"
77 +#include "mir_test_framework/command_line_server_configuration.h"

The first header should be the one that corresponds to the .cpp (which proves the header is self-contained)

~~~~

74 +#include "mir/default_configuration.h"
...
76 +#include "mir/default_server_configuration.h"
...
79 +#include <boost/algorithm/string.hpp>

Unused headers

~~~~

125 +#include "mir_test_framework/command_line_server_configuration.h"
126 #include "mir_test_framework/stubbed_server_configuration.h"

The first header should be the one that corresponds to the .cpp (which proves the header is self-contained)

~~~~

stubbed_server_configuration.cpp no longer needs:

#include "mir/options/default_configuration.h"

It does need "mir/options/configuration.h"