Mir

Merge lp:~josharenson/mir/command_line_config into lp:mir

Proposed by Josh Arenson
Status: Merged
Approved by: Alan Griffiths
Approved revision: no longer in the source branch.
Merged at revision: 1643
Proposed branch: lp:~josharenson/mir/command_line_config
Merge into: lp:mir
Diff against target: 164 lines (+97/-18)
4 files modified
include/test/mir_test_framework/command_line_server_configuration.h (+38/-0)
tests/mir_test_framework/CMakeLists.txt (+1/-0)
tests/mir_test_framework/command_line_server_configuration.cpp (+56/-0)
tests/mir_test_framework/stubbed_server_configuration.cpp (+2/-18)
To merge this branch: bzr merge lp:~josharenson/mir/command_line_config
Reviewer Review Type Date Requested Status
Alberto Aguirre (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Alan Griffiths Approve
Review via email: mp+219619@code.launchpad.net

Commit message

Implements a command_line_server_configuration that allows for creating a DefaultServerConfiguration, but respects command line arguments

Fixes lp:1316987

Description of the change

Implements a command_line_server_configuration that allows for creating a DefaultServerConfiguration, but respects command line arguments

Fixes lp:1316987

To post a comment you must log in.
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

~~~
6 + * Copyright © 2012 Canonical Ltd.
~~~

s/2012/2014

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

Foward declare instead:

namespace mir
{
namespace options
{
class DefaultConfiguration;
}
}

~~~
28 +namespace mo=mir::options;
~~~

I believe we avoid using namespace aliases in headers.

~~~
33 + -> std::shared_ptr<mo::DefaultConfiguration>;
~~~

Missing #include <memory> for shared_ptr

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

Not needed.

~~~
57 + * Copyright © 2012 Canonical Ltd.
~~~

s/2012/2014

~~~
74 +#include "mir/default_configuration.h"
76 +#include "mir/default_server_configuration.h"
83 +#include <memory>
~~~

^-- Not needed

~~~
87 +int argc;
88 +char const** argv;
~~~

Mark them static?

~~~
107 + [](char const* arg) { return !strncmp(arg, "--gtest_", 8); }) - argv;
~~~

Because of the use of strncmp there should be a corresponding #include <cstring>

review: Needs Fixing
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

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"

review: Needs Fixing
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> ~~~
> 87 +int argc;
> 88 +char const** argv;
> ~~~
>
> Mark them static?

No put them in an anonymous namespace (as they were in the "before" code).

review: Needs Fixing
Revision history for this message
Josh Arenson (josharenson) wrote :

>
> ~~~~
>
> 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;

If Configuration is returned, can options be added later (using boost::options)?

Revision history for this message
Alberto Aguirre (albaguirre) wrote :

Just some more minor nits:

~~~
23 +#ifndef COMMAND_LINE_SERVER_CONFIGURATION
~~~

Like Alan mentioned, the convention is:

MIR_TEST_FRAMEWORK_COMMAND_LINE_SERVER_CONFIGURATION_H_

~~~
78 +#include "mir/default_configuration.h"
~~~
Not needed.

~~~
100 + return make_shared<mo::DefaultConfiguration>(::argc, ::argv);
~~~
Per the style guide: http://unity.ubuntu.com/mir/cppguide/index.html?showone=Spaces_vs._Tabs#Spaces_vs._Tabs

It should be indented 4 spaces from the curly bracket.

review: Needs Fixing
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> > The return type should be mir::options::Configuration;
>
>
> If Configuration is returned, can options be added later (using
> boost::options)?

Ack

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

78 +#include "mir/default_configuration.h"

Not used

~~~~

126 +#include "mir_test_framework/command_line_server_configuration.h"
127 #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)

~~~~

96 + using namespace std;

Just use std::make_shared a few lines later.

~~~~

97 + auto configuration_from_commandline()
98 + -> shared_ptr<mo::DefaultConfiguration>
99 + {
100 + return make_shared<mo::DefaultConfiguration>(::argc, ::argv);
101 + }
...
107 + std::cout << "Running main() from " << basename(__FILE__) << std::endl;
108 + ::argc = std::remove_if(
109 + argv,
110 + argv+argc,
111 + [](char const* arg) { return !strncmp(arg, "--gtest_", 8); }) - argv;
112 + ::argv = const_cast<char const**>(argv);
113 +
114 + ::testing::InitGoogleTest(&argc, argv);
115 +
116 + return RUN_ALL_TESTS();

We don't use tabs on this codebase.

~~~~

review: Needs Fixing
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

89 +int argc;
90 +char const** argv;

I'm curious that these are now uninitialized when they were previously initialized with "safe" defaults. Was this intentional? And why?

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Josh Arenson (josharenson) wrote :

> 89 +int argc;
> 90 +char const** argv;
>
> I'm curious that these are now uninitialized when they were previously
> initialized with "safe" defaults. Was this intentional? And why?

Since this _should always_ be called from a command line application, and since a program cannot have more than one main function, argv will always be populated with _at least_ the executable name. I thought it might be confusing to initialize variables that are typically not initialized, however, there isn't any harm in initializing them as such:

132 -char const* dummy[] = {0};
133 -int argc = 0;
134 -char const** argv = dummy;

for safety's sake. Thoughts?

Revision history for this message
Alberto Aguirre (albaguirre) wrote :

LGTM

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== added file 'include/test/mir_test_framework/command_line_server_configuration.h'
--- include/test/mir_test_framework/command_line_server_configuration.h 1970-01-01 00:00:00 +0000
+++ include/test/mir_test_framework/command_line_server_configuration.h 2014-05-16 15:47:38 +0000
@@ -0,0 +1,38 @@
1/*
2 * Copyright © 2014 Canonical Ltd.
3 *
4 * This program is free software: you can redistribute it and/or modify it
5 * under the terms of the GNU General Public License version 3,
6 * as published by the Free Software Foundation.
7 *
8 * This program is distributed in the hope that it will be useful,
9 * but WITHOUT ANY WARRANTY; without even the implied warranty of
10 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11 * GNU General Public License for more details.
12 *
13 * You should have received a copy of the GNU General Public License
14 * along with this program. If not, see <http://www.gnu.org/licenses/>.
15 *
16 * Authored by: Josh Arenson <joshua.arenson@canonical.com>
17 */
18
19#ifndef MIR_TEST_FRAMEWORK_COMMAND_LINE_SERVER_CONFIGURATION
20#define MIR_TEST_FRAMEWORK_COMMAND_LINE_SERVER_CONFIGURATION
21
22#include <memory>
23
24namespace mir
25{
26namespace options
27{
28class DefaultConfiguration;
29}
30}
31
32namespace mir_test_framework
33{
34 auto configuration_from_commandline()
35 -> std::shared_ptr<mir::options::DefaultConfiguration>;
36}
37
38#endif /* MIR_TEST_FRAMEWORK_COMMAND_LINE_SERVER_CONFIGURATION */
039
=== modified file 'tests/mir_test_framework/CMakeLists.txt'
--- tests/mir_test_framework/CMakeLists.txt 2014-05-14 06:34:25 +0000
+++ tests/mir_test_framework/CMakeLists.txt 2014-05-16 15:47:38 +0000
@@ -8,6 +8,7 @@
8set(8set(
9 TEST_FRAMEWORK_SRCS9 TEST_FRAMEWORK_SRCS
1010
11 command_line_server_configuration.cpp
11 cross_process_sync.cpp12 cross_process_sync.cpp
12 in_process_server.cpp13 in_process_server.cpp
13 testing_server_options.cpp14 testing_server_options.cpp
1415
=== added file 'tests/mir_test_framework/command_line_server_configuration.cpp'
--- tests/mir_test_framework/command_line_server_configuration.cpp 1970-01-01 00:00:00 +0000
+++ tests/mir_test_framework/command_line_server_configuration.cpp 2014-05-16 15:47:38 +0000
@@ -0,0 +1,56 @@
1/*
2 * Copyright © 2014 Canonical Ltd.
3 *
4 * This program is free software: you can redistribute it and/or modify it
5 * under the terms of the GNU General Public License version 3,
6 * as published by the Free Software Foundation.
7 *
8 * This program is distributed in the hope that it will be useful,
9 * but WITHOUT ANY WARRANTY; without even the implied warranty of
10 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11 * GNU General Public License for more details.
12 *
13 * You should have received a copy of the GNU General Public License
14 * along with this program. If not, see <http://www.gnu.org/licenses/>.
15 *
16 */
17
18#include "mir_test_framework/command_line_server_configuration.h"
19#include "mir/options/default_configuration.h"
20
21#include <gtest/gtest.h>
22
23#include <algorithm>
24#include <cstring>
25
26namespace mo=mir::options;
27
28namespace
29{
30int argc;
31char const** argv;
32}
33
34namespace mir_test_framework
35{
36 auto configuration_from_commandline()
37 -> std::shared_ptr<mo::DefaultConfiguration>
38 {
39 return std::make_shared<mo::DefaultConfiguration>(::argc, ::argv);
40 }
41}
42
43int main(int argc, char** argv)
44{
45 // Override this standard gtest message
46 std::cout << "Running main() from " << basename(__FILE__) << std::endl;
47 ::argc = std::remove_if(
48 argv,
49 argv+argc,
50 [](char const* arg) { return !strncmp(arg, "--gtest_", 8); }) - argv;
51 ::argv = const_cast<char const**>(argv);
52
53 ::testing::InitGoogleTest(&argc, argv);
54
55 return RUN_ALL_TESTS();
56}
057
=== modified file 'tests/mir_test_framework/stubbed_server_configuration.cpp'
--- tests/mir_test_framework/stubbed_server_configuration.cpp 2014-05-12 04:29:42 +0000
+++ tests/mir_test_framework/stubbed_server_configuration.cpp 2014-05-16 15:47:38 +0000
@@ -17,6 +17,7 @@
17 */17 */
1818
19#include "mir_test_framework/stubbed_server_configuration.h"19#include "mir_test_framework/stubbed_server_configuration.h"
20#include "mir_test_framework/command_line_server_configuration.h"
2021
21#include "mir/options/default_configuration.h"22#include "mir/options/default_configuration.h"
22#include "mir/geometry/rectangle.h"23#include "mir/geometry/rectangle.h"
@@ -58,10 +59,6 @@
5859
59namespace60namespace
60{61{
61char const* dummy[] = {0};
62int argc = 0;
63char const** argv = dummy;
64
65class StubFDBuffer : public mtd::StubBuffer62class StubFDBuffer : public mtd::StubBuffer
66{63{
67public:64public:
@@ -222,7 +219,7 @@
222mtf::StubbedServerConfiguration::StubbedServerConfiguration() :219mtf::StubbedServerConfiguration::StubbedServerConfiguration() :
223 DefaultServerConfiguration([]220 DefaultServerConfiguration([]
224 {221 {
225 auto result = std::make_shared<mo::DefaultConfiguration>(::argc, ::argv);222 auto result = mtf::configuration_from_commandline();
226223
227 namespace po = boost::program_options;224 namespace po = boost::program_options;
228225
@@ -278,16 +275,3 @@
278 else275 else
279 return std::make_shared<mi::NullInputDispatcherConfiguration>();276 return std::make_shared<mi::NullInputDispatcherConfiguration>();
280}277}
281
282int main(int argc, char** argv)
283{
284 ::argc = std::remove_if(
285 argv,
286 argv+argc,
287 [](char const* arg) { return !strncmp(arg, "--gtest_", 8); }) - argv;
288 ::argv = const_cast<char const**>(argv);
289
290 ::testing::InitGoogleTest(&argc, argv);
291
292 return RUN_ALL_TESTS();
293}

Subscribers

People subscribed via source and target branches