Mir

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

Proposed by Daniel van Vugt
Status: Merged
Approved by: Alexandros Frantzis
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
examples/demo-inprocess-surface-client/example_egl_helper.cpp (+1/-0)
examples/demo-inprocess-surface-client/inprocess_egl_client.cpp (+2/-0)
examples/demo_client_accelerated.cpp (+2/-0)
src/server/options/program_option.cpp (+1/-0)
tests/death-tests/test_process_death.cpp (+6/-0)
To merge this branch: bzr merge lp:~vanvugt/mir/saucy-build
Reviewer Review Type Date Requested Status
Alexandros Frantzis (community) Approve
Alan Griffiths Approve
PS Jenkins bot (community) continuous-integration Approve
Chris Halse Rogers Approve
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.
Revision history for this message
Chris Halse Rogers (raof) wrote :

LCTM.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
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
Revision history for this message
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
Revision history for this message
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
Revision history for this message
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

[H/L] Next/Prev Comment, [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