Mir

Merge lp:~vanvugt/mir/saucy-build into lp:~mir-team/mir/trunk

Proposed by Daniel van Vugt on 2013-06-13
Status: Merged
Approved by: Alexandros Frantzis on 2013-06-13
Approved revision: 739
Merged at revision: 739
Proposed branch: lp:~vanvugt/mir/saucy-build
Merge into: lp:~mir-team/mir/trunk
Diff against target: 68 lines (+12/-0) 5 files modified
To merge this branch: bzr merge lp:~vanvugt/mir/saucy-build
Reviewer Review Type Date Requested Status
Alexandros Frantzis Approve on 2013-06-13
Alan Griffiths Approve on 2013-06-13
PS Jenkins bot continuous-integration Approve on 2013-06-13
Chris Halse Rogers 2013-06-13 Approve on 2013-06-13
Review via email: mp+169108@code.launchpad.net

Commit Message

Fix debuild failures on saucy / gcc-4.8.

To post a comment you must log in.
Chris Halse Rogers (raof) wrote :

LCTM.

review: Approve
Alexandros Frantzis (afrantzis) wrote :

58 +#ifdef NDEBUG

Instead of disabling the test I would propose replacing the assert() in the MirTestFramework contstructor with "if (...) abort()" as a temporary measure (like in Stephen's branch).

However, replacing all assert()s with our own variant which doesn't care about NDEBUG (the "Right Way"), doesn't seem to require too much effort, so why not go for it?

review: Needs Fixing
Alan Griffiths (alan-griffiths) wrote :

> 58 +#ifdef NDEBUG
>
> Instead of disabling the test I would propose replacing the assert() in the

This is something I debated with Thomas when he added test like this.

/1/ A test represents a requirement on the behavior of code.

/2/ A precondition is something that code is allowed to assume (as it is the responsibility of the caller to ensure it holds).

/3/ The code has no responsibility to behave in a particular way if preconditions are violated. Its behavior is unspecified (or even undefined).

/4/ Tests that "preconditions violations" are reported correctly imply that they are not really preconditions - just conditions.

The point: *if* we want some preconditions to be conditions in debug builds, then it makes sense for the corresponding tests to be conditionally compiled.

Personally, I don't think these checks should ever be conditions *with tests to validate the behavior*. But unless there are performance reasons I feel that is is as important to aid problem diagnosis in release builds as in debug ones.

review: Approve
Alexandros Frantzis (afrantzis) wrote :

> The point: *if* we want some preconditions to be conditions in debug builds,
> then it makes sense for the corresponding tests to be conditionally compiled.
>
> Personally, I don't think these checks should ever be conditions *with tests
> to validate the behavior*. But unless there are performance reasons I feel
> that is is as important to aid problem diagnosis in release builds as in debug
> ones.

OK, I am convinced. My preference would be to remove the test, but I can live with it for now.

review: Approve
Daniel van Vugt (vanvugt) wrote :

If our error handling strategy depends on assert never being eliminated then that's obviously bad, but a discussion for elsewhere.

I'm not proposing any changes here /directly/ relevant to that discussion.

Preview Diff

[J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'examples/demo-inprocess-surface-client/example_egl_helper.cpp'
2--- examples/demo-inprocess-surface-client/example_egl_helper.cpp 2013-05-07 19:03:36 +0000
3+++ examples/demo-inprocess-surface-client/example_egl_helper.cpp 2013-06-13 06:57:27 +0000
4@@ -46,6 +46,7 @@
5 rc = eglChooseConfig(display, attribs, &egl_config, 1, &n);
6 assert(rc == EGL_TRUE);
7 assert(n == 1);
8+ (void)rc;
9
10 surface = eglCreateWindowSurface(display, egl_config, native_window, nullptr);
11 assert(surface != EGL_NO_SURFACE);
12
13=== modified file 'examples/demo-inprocess-surface-client/inprocess_egl_client.cpp'
14--- examples/demo-inprocess-surface-client/inprocess_egl_client.cpp 2013-05-21 17:16:43 +0000
15+++ examples/demo-inprocess-surface-client/inprocess_egl_client.cpp 2013-06-13 06:57:27 +0000
16@@ -111,6 +111,8 @@
17 gl_animation.step();
18 }
19
20+ (void)rc;
21+
22 input_thread->stop();
23 ///\internal [loop_tag]
24 }
25
26=== modified file 'examples/demo_client_accelerated.cpp'
27--- examples/demo_client_accelerated.cpp 2013-05-02 00:11:18 +0000
28+++ examples/demo_client_accelerated.cpp 2013-06-13 06:57:27 +0000
29@@ -143,6 +143,8 @@
30 mir_connection_release(connection);
31 puts("Connection released");
32
33+ (void)rc;
34+
35 return 0;
36 }
37
38
39=== modified file 'src/server/options/program_option.cpp'
40--- src/server/options/program_option.cpp 2013-04-30 23:28:00 +0000
41+++ src/server/options/program_option.cpp 2013-06-13 06:57:27 +0000
42@@ -21,6 +21,7 @@
43 #include <boost/program_options/parsers.hpp>
44
45 #include <fstream>
46+#include <iostream>
47
48 namespace mo = mir::options;
49 namespace po = boost::program_options;
50
51=== modified file 'tests/death-tests/test_process_death.cpp'
52--- tests/death-tests/test_process_death.cpp 2013-05-10 00:39:29 +0000
53+++ tests/death-tests/test_process_death.cpp 2013-06-13 06:57:27 +0000
54@@ -20,8 +20,14 @@
55
56 #include <gtest/gtest.h>
57
58+#ifdef NDEBUG
59+// If NDEBUG is defined then assert() does nothing. So this test would fail.
60+TEST(ProcessDeathTest,
61+ DISABLED_construction_with_an_invalid_pid_triggers_assertion)
62+#else
63 TEST(ProcessDeathTest,
64 construction_with_an_invalid_pid_triggers_assertion)
65+#endif
66 {
67 EXPECT_EXIT(
68 mir_test_framework::Process p(0),

Subscribers

People subscribed via source and target branches