Merge lp:~josharenson/mir/command_line_config into lp:mir
- command_line_config
- Merge into development-branch
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 |
Related bugs: |
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_
Fixes lp:1316987
Description of the change
Implements a command_
Fixes lp:1316987
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1622
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Alan Griffiths (alan-griffiths) wrote : | # |
23 +#ifndef COMMAND_
24 +#define COMMAND_
Missing MIR_TEST_FRAMEWORK_ prefix
~~~~
26 +#include "mir/options/
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_
33 + -> std::shared_
The return type should be mir::options:
~~~~
36 +int main(int argc, char** argv);
not wanted.
~~~~
74 +#include "mir/default_
75 +#include "mir/options/
76 +#include "mir/default_
77 +#include "mir_test_
The first header should be the one that corresponds to the .cpp (which proves the header is self-contained)
~~~~
74 +#include "mir/default_
...
76 +#include "mir/default_
...
79 +#include <boost/
Unused headers
~~~~
125 +#include "mir_test_
126 #include "mir_test_
The first header should be the one that corresponds to the .cpp (which proves the header is self-contained)
~~~~
stubbed_
#include "mir/options/
It does need "mir/options/
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).
Josh Arenson (josharenson) wrote : | # |
>
> ~~~~
>
> 30 +namespace mir_test_framework
> 31 +{
> 32 + auto configuration_
> 33 + -> std::shared_
>
> The return type should be mir::options:
If Configuration is returned, can options be added later (using boost::options)?
Alberto Aguirre (albaguirre) wrote : | # |
Just some more minor nits:
~~~
23 +#ifndef COMMAND_
~~~
Like Alan mentioned, the convention is:
MIR_TEST_
~~~
78 +#include "mir/default_
~~~
Not needed.
~~~
100 + return make_shared<
~~~
Per the style guide: http://
It should be indented 4 spaces from the curly bracket.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1626
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1627
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Alan Griffiths (alan-griffiths) wrote : | # |
> > The return type should be mir::options:
>
>
> If Configuration is returned, can options be added later (using
> boost::options)?
Ack
Alan Griffiths (alan-griffiths) wrote : | # |
78 +#include "mir/default_
Not used
~~~~
126 +#include "mir_test_
127 #include "mir_test_
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_
98 + -> shared_
99 + {
100 + return make_shared<
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:
115 +
116 + return RUN_ALL_TESTS();
We don't use tabs on this codebase.
~~~~
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1628
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
FAILURE: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
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?
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1628
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
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?
Preview Diff
1 | === added file 'include/test/mir_test_framework/command_line_server_configuration.h' | |||
2 | --- include/test/mir_test_framework/command_line_server_configuration.h 1970-01-01 00:00:00 +0000 | |||
3 | +++ include/test/mir_test_framework/command_line_server_configuration.h 2014-05-16 15:47:38 +0000 | |||
4 | @@ -0,0 +1,38 @@ | |||
5 | 1 | /* | ||
6 | 2 | * Copyright © 2014 Canonical Ltd. | ||
7 | 3 | * | ||
8 | 4 | * This program is free software: you can redistribute it and/or modify it | ||
9 | 5 | * under the terms of the GNU General Public License version 3, | ||
10 | 6 | * as published by the Free Software Foundation. | ||
11 | 7 | * | ||
12 | 8 | * This program is distributed in the hope that it will be useful, | ||
13 | 9 | * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
14 | 10 | * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
15 | 11 | * GNU General Public License for more details. | ||
16 | 12 | * | ||
17 | 13 | * You should have received a copy of the GNU General Public License | ||
18 | 14 | * along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
19 | 15 | * | ||
20 | 16 | * Authored by: Josh Arenson <joshua.arenson@canonical.com> | ||
21 | 17 | */ | ||
22 | 18 | |||
23 | 19 | #ifndef MIR_TEST_FRAMEWORK_COMMAND_LINE_SERVER_CONFIGURATION | ||
24 | 20 | #define MIR_TEST_FRAMEWORK_COMMAND_LINE_SERVER_CONFIGURATION | ||
25 | 21 | |||
26 | 22 | #include <memory> | ||
27 | 23 | |||
28 | 24 | namespace mir | ||
29 | 25 | { | ||
30 | 26 | namespace options | ||
31 | 27 | { | ||
32 | 28 | class DefaultConfiguration; | ||
33 | 29 | } | ||
34 | 30 | } | ||
35 | 31 | |||
36 | 32 | namespace mir_test_framework | ||
37 | 33 | { | ||
38 | 34 | auto configuration_from_commandline() | ||
39 | 35 | -> std::shared_ptr<mir::options::DefaultConfiguration>; | ||
40 | 36 | } | ||
41 | 37 | |||
42 | 38 | #endif /* MIR_TEST_FRAMEWORK_COMMAND_LINE_SERVER_CONFIGURATION */ | ||
43 | 0 | 39 | ||
44 | === modified file 'tests/mir_test_framework/CMakeLists.txt' | |||
45 | --- tests/mir_test_framework/CMakeLists.txt 2014-05-14 06:34:25 +0000 | |||
46 | +++ tests/mir_test_framework/CMakeLists.txt 2014-05-16 15:47:38 +0000 | |||
47 | @@ -8,6 +8,7 @@ | |||
48 | 8 | set( | 8 | set( |
49 | 9 | TEST_FRAMEWORK_SRCS | 9 | TEST_FRAMEWORK_SRCS |
50 | 10 | 10 | ||
51 | 11 | command_line_server_configuration.cpp | ||
52 | 11 | cross_process_sync.cpp | 12 | cross_process_sync.cpp |
53 | 12 | in_process_server.cpp | 13 | in_process_server.cpp |
54 | 13 | testing_server_options.cpp | 14 | testing_server_options.cpp |
55 | 14 | 15 | ||
56 | === added file 'tests/mir_test_framework/command_line_server_configuration.cpp' | |||
57 | --- tests/mir_test_framework/command_line_server_configuration.cpp 1970-01-01 00:00:00 +0000 | |||
58 | +++ tests/mir_test_framework/command_line_server_configuration.cpp 2014-05-16 15:47:38 +0000 | |||
59 | @@ -0,0 +1,56 @@ | |||
60 | 1 | /* | ||
61 | 2 | * Copyright © 2014 Canonical Ltd. | ||
62 | 3 | * | ||
63 | 4 | * This program is free software: you can redistribute it and/or modify it | ||
64 | 5 | * under the terms of the GNU General Public License version 3, | ||
65 | 6 | * as published by the Free Software Foundation. | ||
66 | 7 | * | ||
67 | 8 | * This program is distributed in the hope that it will be useful, | ||
68 | 9 | * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
69 | 10 | * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
70 | 11 | * GNU General Public License for more details. | ||
71 | 12 | * | ||
72 | 13 | * You should have received a copy of the GNU General Public License | ||
73 | 14 | * along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
74 | 15 | * | ||
75 | 16 | */ | ||
76 | 17 | |||
77 | 18 | #include "mir_test_framework/command_line_server_configuration.h" | ||
78 | 19 | #include "mir/options/default_configuration.h" | ||
79 | 20 | |||
80 | 21 | #include <gtest/gtest.h> | ||
81 | 22 | |||
82 | 23 | #include <algorithm> | ||
83 | 24 | #include <cstring> | ||
84 | 25 | |||
85 | 26 | namespace mo=mir::options; | ||
86 | 27 | |||
87 | 28 | namespace | ||
88 | 29 | { | ||
89 | 30 | int argc; | ||
90 | 31 | char const** argv; | ||
91 | 32 | } | ||
92 | 33 | |||
93 | 34 | namespace mir_test_framework | ||
94 | 35 | { | ||
95 | 36 | auto configuration_from_commandline() | ||
96 | 37 | -> std::shared_ptr<mo::DefaultConfiguration> | ||
97 | 38 | { | ||
98 | 39 | return std::make_shared<mo::DefaultConfiguration>(::argc, ::argv); | ||
99 | 40 | } | ||
100 | 41 | } | ||
101 | 42 | |||
102 | 43 | int main(int argc, char** argv) | ||
103 | 44 | { | ||
104 | 45 | // Override this standard gtest message | ||
105 | 46 | std::cout << "Running main() from " << basename(__FILE__) << std::endl; | ||
106 | 47 | ::argc = std::remove_if( | ||
107 | 48 | argv, | ||
108 | 49 | argv+argc, | ||
109 | 50 | [](char const* arg) { return !strncmp(arg, "--gtest_", 8); }) - argv; | ||
110 | 51 | ::argv = const_cast<char const**>(argv); | ||
111 | 52 | |||
112 | 53 | ::testing::InitGoogleTest(&argc, argv); | ||
113 | 54 | |||
114 | 55 | return RUN_ALL_TESTS(); | ||
115 | 56 | } | ||
116 | 0 | 57 | ||
117 | === modified file 'tests/mir_test_framework/stubbed_server_configuration.cpp' | |||
118 | --- tests/mir_test_framework/stubbed_server_configuration.cpp 2014-05-12 04:29:42 +0000 | |||
119 | +++ tests/mir_test_framework/stubbed_server_configuration.cpp 2014-05-16 15:47:38 +0000 | |||
120 | @@ -17,6 +17,7 @@ | |||
121 | 17 | */ | 17 | */ |
122 | 18 | 18 | ||
123 | 19 | #include "mir_test_framework/stubbed_server_configuration.h" | 19 | #include "mir_test_framework/stubbed_server_configuration.h" |
124 | 20 | #include "mir_test_framework/command_line_server_configuration.h" | ||
125 | 20 | 21 | ||
126 | 21 | #include "mir/options/default_configuration.h" | 22 | #include "mir/options/default_configuration.h" |
127 | 22 | #include "mir/geometry/rectangle.h" | 23 | #include "mir/geometry/rectangle.h" |
128 | @@ -58,10 +59,6 @@ | |||
129 | 58 | 59 | ||
130 | 59 | namespace | 60 | namespace |
131 | 60 | { | 61 | { |
132 | 61 | char const* dummy[] = {0}; | ||
133 | 62 | int argc = 0; | ||
134 | 63 | char const** argv = dummy; | ||
135 | 64 | |||
136 | 65 | class StubFDBuffer : public mtd::StubBuffer | 62 | class StubFDBuffer : public mtd::StubBuffer |
137 | 66 | { | 63 | { |
138 | 67 | public: | 64 | public: |
139 | @@ -222,7 +219,7 @@ | |||
140 | 222 | mtf::StubbedServerConfiguration::StubbedServerConfiguration() : | 219 | mtf::StubbedServerConfiguration::StubbedServerConfiguration() : |
141 | 223 | DefaultServerConfiguration([] | 220 | DefaultServerConfiguration([] |
142 | 224 | { | 221 | { |
144 | 225 | auto result = std::make_shared<mo::DefaultConfiguration>(::argc, ::argv); | 222 | auto result = mtf::configuration_from_commandline(); |
145 | 226 | 223 | ||
146 | 227 | namespace po = boost::program_options; | 224 | namespace po = boost::program_options; |
147 | 228 | 225 | ||
148 | @@ -278,16 +275,3 @@ | |||
149 | 278 | else | 275 | else |
150 | 279 | return std::make_shared<mi::NullInputDispatcherConfiguration>(); | 276 | return std::make_shared<mi::NullInputDispatcherConfiguration>(); |
151 | 280 | } | 277 | } |
152 | 281 | |||
153 | 282 | int main(int argc, char** argv) | ||
154 | 283 | { | ||
155 | 284 | ::argc = std::remove_if( | ||
156 | 285 | argv, | ||
157 | 286 | argv+argc, | ||
158 | 287 | [](char const* arg) { return !strncmp(arg, "--gtest_", 8); }) - argv; | ||
159 | 288 | ::argv = const_cast<char const**>(argv); | ||
160 | 289 | |||
161 | 290 | ::testing::InitGoogleTest(&argc, argv); | ||
162 | 291 | |||
163 | 292 | return RUN_ALL_TESTS(); | ||
164 | 293 | } |
~~~
6 + * Copyright © 2012 Canonical Ltd.
~~~
s/2012/2014
~~~ default_ configuration. h"
26 +#include "mir/options/
~~~
Foward declare instead:
namespace mir ation;
{
namespace options
{
class DefaultConfigur
}
}
~~~
28 +namespace mo=mir::options;
~~~
I believe we avoid using namespace aliases in headers.
~~~ ptr<mo: :DefaultConfigu ration> ;
33 + -> std::shared_
~~~
Missing #include <memory> for shared_ptr
~~~
36 +int main(int argc, char** argv);
~~~
Not needed.
~~~
57 + * Copyright © 2012 Canonical Ltd.
~~~
s/2012/2014
~~~ configuration. h" server_ configuration. h"
74 +#include "mir/default_
76 +#include "mir/default_
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>