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

[Daniel van Vugt]

Fix debuild failures on saucy / gcc-4.8.

[Chris Halse Rogers (raof)]

LCTM.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:739
http://jenkins.qa.ubuntu.com/job/mir-ci/720/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/894
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/776
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/205
        deb: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/205/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/409

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/720/rebuild

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

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)]

> 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)]

> 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)]

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.

=== modified file 'examples/demo-inprocess-surface-client/example_egl_helper.cpp'
--- examples/demo-inprocess-surface-client/example_egl_helper.cpp	2013-05-07 19:03:36 +0000
+++ examples/demo-inprocess-surface-client/example_egl_helper.cpp	2013-06-13 06:57:27 +0000
@@ -46,6 +46,7 @@
     rc = eglChooseConfig(display, attribs, &egl_config, 1, &n);
     assert(rc == EGL_TRUE);
     assert(n == 1);
+    (void)rc;
 
     surface = eglCreateWindowSurface(display, egl_config, native_window, nullptr);
     assert(surface != EGL_NO_SURFACE);

=== modified file 'examples/demo-inprocess-surface-client/inprocess_egl_client.cpp'
--- examples/demo-inprocess-surface-client/inprocess_egl_client.cpp	2013-05-21 17:16:43 +0000
+++ examples/demo-inprocess-surface-client/inprocess_egl_client.cpp	2013-06-13 06:57:27 +0000
@@ -111,6 +111,8 @@
         gl_animation.step();
     }
 
+    (void)rc;
+
     input_thread->stop();
     ///\internal [loop_tag]
 }

=== modified file 'examples/demo_client_accelerated.cpp'
--- examples/demo_client_accelerated.cpp	2013-05-02 00:11:18 +0000
+++ examples/demo_client_accelerated.cpp	2013-06-13 06:57:27 +0000
@@ -143,6 +143,8 @@
     mir_connection_release(connection);
     puts("Connection released");
 
+    (void)rc;
+
     return 0;
 }
 

=== modified file 'src/server/options/program_option.cpp'
--- src/server/options/program_option.cpp	2013-04-30 23:28:00 +0000
+++ src/server/options/program_option.cpp	2013-06-13 06:57:27 +0000
@@ -21,6 +21,7 @@
 #include <boost/program_options/parsers.hpp>
 
 #include <fstream>
+#include <iostream>
 
 namespace mo = mir::options;
 namespace po = boost::program_options;

=== modified file 'tests/death-tests/test_process_death.cpp'
--- tests/death-tests/test_process_death.cpp	2013-05-10 00:39:29 +0000
+++ tests/death-tests/test_process_death.cpp	2013-06-13 06:57:27 +0000
@@ -20,8 +20,14 @@
 
 #include <gtest/gtest.h>
 
+#ifdef NDEBUG
+// If NDEBUG is defined then assert() does nothing. So this test would fail.
+TEST(ProcessDeathTest,
+     DISABLED_construction_with_an_invalid_pid_triggers_assertion)
+#else
 TEST(ProcessDeathTest,
      construction_with_an_invalid_pid_triggers_assertion)
+#endif
 {
     EXPECT_EXIT(
         mir_test_framework::Process p(0),