Merge lp:~raof/mir/optional-check-precondition into lp:mir

[Chris Halse Rogers]

Make optional_value<>::value() check its precondition.

Namely, that a value has been set.

Then fix the place in the code where we access an unset optional_value.

Make optional_value<>::value() check its precondition.

Namely, that a value has been set.

This then breaks all the acceptance tests, because the MirSurface constructor accesses the value of an unset optional_value, so fix that.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2605
http://jenkins.qa.ubuntu.com/job/mir-ci/3956/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2641
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/152
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2589
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/112
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/112/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2589
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2589/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5439
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20856

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

*Needs Discussion*

I'd prefer a mir::fatal_error:

20 + if (!is_set())
21 + {
22 + BOOST_THROW_EXCEPTION(std::logic_error("Accessing value of unset optional_value"));
23 + }

As you say, it is a *precondition* failure and exceptions are for *postcondition* failures.

8 +#include <boost/throw_exception.hpp>

This pulls quite a lot into what was previously a lightweight include.

review: Needs Information

[Chris Halse Rogers (raof)]

FWIW, std::experimental::optional<>::value() throws bad_optional_access in the case of a disengaged optional value.

I'd also be happy with mir::fatal_error; optional<>::operator*() invokes undefined behaviour in the case of a disengaged optional value.

The throw_exception.hpp include is indeed pretty big, and I don't think any of our other public headers include it. I'm happy to just throw, if it's deemed excessive.

[Kevin DuBois (kdub)]

I'd be more in favor of throwing like std::experimental::optional does, just in the name of not bringing down the server (esp as optional is used a fair amount in client requests, which would just fail on that client's request and recover).

[Kevin DuBois (kdub)]

I guess approve, although I also would prefer not pulling in boost and just throwing a std exception type.

review: Approve

[Alan Griffiths (alan-griffiths)]

I'm more concerned that we should detect and correct invalid logic during testing than have it accidentally hidden a catch clause intended for a different case. So I'd prefer being able to abort. fatal_error gives that option:

/**
 * fatal_error() is strictly for "this should never happen" situations that
 * you cannot recover from. By default it points at fatal_error_except().
 * Note the reason parameter is a simple char* so its value is clearly visible
 * in stack trace output.
 * \param [in] reason A printf-style format string.
 */

...

  --on-fatal-error-abort On "fatal error" conditions [e.g.
                                        drivers behaving in unexpected ways]
                                        abort (to get a core dump)

review: Needs Fixing

[Alberto Aguirre (albaguirre)]

>I'm more concerned that we should detect and correct invalid logic during testing

+1 I also prefer fatal_error for this case.

review: Needs Fixing

[Kevin DuBois (kdub)]

> I'm more concerned that we should detect and correct invalid logic during
> testing than have it accidentally hidden a catch clause intended for a
> different case. So I'd prefer being able to abort. fatal_error gives that
> option:
Ah, had forgotten about the option to have a nicer fatal error... with that fatal_error makes more sense to me too

[lp:~raof/mir/optional-check-precondition]

2606. By Chris Halse Rogers on 2015-06-04

Move mir::fatal_error to mircommon

2607. By Chris Halse Rogers on 2015-06-04

Use mir::fatal_error rather than throwing an exception for optional_value precondition violations

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2607
http://jenkins.qa.ubuntu.com/job/mir-ci/4003/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2704
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/217
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2652/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/159
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/159/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2652
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2652/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5497/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20954

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

review: Needs Fixing (continuous-integration)

[Alberto Aguirre (albaguirre)]

 +# Symbols moved from libmirplatform; can be folded into a MIR_COMMON stanza
66 +# at next mirplatform ABI break.
67 +MIRPLATFORM_7 {
68 + global:
69 + extern "C++" {
70 + mir::fatal_error*;
71 + mir::fatal_error_abort*;
72 + mir::fatal_error_except*;
73 + };
74 +};

We already broke MIRPLATFORM ABI :)

review: Needs Fixing

[lp:~raof/mir/optional-check-precondition]

2608. By Chris Halse Rogers on 2015-06-09

Merge trunk

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2608
http://jenkins.qa.ubuntu.com/job/mir-ci/4041/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2763
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/276
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2711/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/197
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/197/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2711
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2711/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5551/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21031

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

review: Needs Fixing (continuous-integration)

[Alberto Aguirre (albaguirre)]

^--
Looks like lp:1463315

LGTM.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2608
http://jenkins.qa.ubuntu.com/job/mir-ci/4049/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2773
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/286
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2721
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/205
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/205/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2721
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2721/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5561
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21053

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

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

Alan's Needs fixing has been addressed. TA'ing.

=== renamed file 'include/platform/mir/fatal.h' => 'include/common/mir/fatal.h'
=== modified file 'include/common/mir/optional_value.h'
--- include/common/mir/optional_value.h	2015-05-07 09:06:20 +0000
+++ include/common/mir/optional_value.h	2015-06-09 07:41:50 +0000
@@ -19,6 +19,8 @@
 #ifndef MIR_OPTIONAL_VALUE_H_
 #define MIR_OPTIONAL_VALUE_H_
 
+#include "mir/fatal.h"
+
 namespace mir
 {
 template<typename T>
@@ -36,7 +38,14 @@
     }
 
     bool is_set() const { return is_set_; }
-    T value() const { return value_; }
+    T value() const
+    {
+        if (!is_set())
+        {
+            (*fatal_error)("Accessing value of unset optional");
+        }
+        return value_;
+    }
 
 private:
     T value_;

=== modified file 'src/client/mir_surface.cpp'
--- src/client/mir_surface.cpp	2015-05-21 19:25:51 +0000
+++ src/client/mir_surface.cpp	2015-06-09 07:41:50 +0000
@@ -145,7 +145,7 @@
     mir_surface_callback callback, void * context)
     : server{&the_server},
       debug{debug},
-      name{spec.surface_name.value()},
+      name{spec.surface_name.is_set() ? spec.surface_name.value() : ""},
       connection(allocating_connection),
       buffer_stream_factory(buffer_stream_factory),
       input_platform(input_platform),

=== modified file 'src/common/CMakeLists.txt'
--- src/common/CMakeLists.txt	2015-06-02 15:07:18 +0000
+++ src/common/CMakeLists.txt	2015-06-09 07:41:50 +0000
@@ -17,9 +17,11 @@
 add_subdirectory(thread)
 add_subdirectory(time)
 add_subdirectory(dispatch)
+add_subdirectory(fatal)
 
 list(APPEND MIR_COMMON_SOURCES
   $<TARGET_OBJECTS:mirtime>
+  $<TARGET_OBJECTS:mirfatal>
   ${CMAKE_CURRENT_SOURCE_DIR}/log.cpp
   )
 

=== renamed directory 'src/platform/fatal' => 'src/common/fatal'
=== modified file 'src/common/symbols.map'
--- src/common/symbols.map	2015-06-02 15:07:18 +0000
+++ src/common/symbols.map	2015-06-09 07:41:50 +0000
@@ -1,4 +1,3 @@
-
 MIR_COMMON_5 {
  global: mir_*;
   extern "C++" {
@@ -28,6 +27,9 @@
     mir::dispatch::ThreadedDispatcher::?ThreadedDispatcher*;
     mir::dispatch::ThreadedDispatcher::add_thread*;
     mir::dispatch::ThreadedDispatcher::remove_thread*;
+    mir::fatal_error*;
+    mir::fatal_error_abort*;
+    mir::fatal_error_except*;
     mir::Fd::Fd*;
     mir::Fd::invalid*;
     mir::Fd::operator*;

=== modified file 'src/platform/CMakeLists.txt'
--- src/platform/CMakeLists.txt	2015-03-31 02:35:42 +0000
+++ src/platform/CMakeLists.txt	2015-06-09 07:41:50 +0000
@@ -13,7 +13,6 @@
 set(MIR_PLATFORM_OBJECTS
   $<TARGET_OBJECTS:mirplatformgraphicscommon>
   $<TARGET_OBJECTS:miroptions>
-  $<TARGET_OBJECTS:mirfatal>
   $<TARGET_OBJECTS:mirudev>
 )
 
@@ -24,7 +23,6 @@
 
 add_subdirectory(graphics/)
 add_subdirectory(options)
-add_subdirectory(fatal)
 add_subdirectory(udev)
 
 set(MIR_PLATFORM_OBJECTS ${MIR_PLATFORM_OBJECTS} PARENT_SCOPE)

=== modified file 'src/platform/symbols.map'
--- src/platform/symbols.map	2015-05-19 21:34:34 +0000
+++ src/platform/symbols.map	2015-06-09 07:41:50 +0000
@@ -3,9 +3,6 @@
   extern "C++" {
 # The following symbols come from running a script over the generated docs. Vis:
 # ../tools/process_doxygen_xml.py doc/xml/*.xml | grep "^mirplatform public" | sed "s/mirplatform public: /    /" | sort  
-    mir::fatal_error*;
-    mir::fatal_error_abort*;
-    mir::fatal_error_except*;
     mir::graphics::BufferBasic::BufferBasic*;
     mir::graphics::Buffer::Buffer*;
     mir::graphics::Buffer::gl_bind_to_texture*;