Merge lp:~thomas-voss/properties-cpp/cleanup into lp:properties-cpp

[Thomas Voß]

Adjusted the introductory code example to always drain the event queue.
Added a test to make sure that a signal resets all registered slots on destruction.
Documented the usage of a shared private state.

Adjusted the introductory code example to always drain the event queue.
Added a test to make sure that a signal resets all registered slots on destruction.
Documented the usage of a shared private state.

[Jussi Pakkanen (jpakkane)]

Looks good. There are a few cases of for(auto foo : ...) which could be for(auto &foo : ...) but the performance impact is probably unnoticeable.

review: Approve

=== modified file 'README.md'
--- README.md	2013-11-13 10:41:06 +0000
+++ README.md	2013-11-27 08:46:56 +0000
@@ -72,8 +72,11 @@
                         std::chrono::milliseconds{500},
                         [this]() { return handlers.size() > 0; });
 
-            handlers.front()();
-            handlers.pop();
+            while (handlers.size() > 0)
+            {
+                handlers.front()();
+                handlers.pop();
+            }
         }
     }
 
@@ -131,4 +134,4 @@
     if (dispatcher_thread.joinable())
         dispatcher_thread.join();
 }
-~~~~~~~~~~~~~
\ No newline at end of file
+~~~~~~~~~~~~~

=== modified file 'include/com/ubuntu/connection.h'
--- include/com/ubuntu/connection.h	2013-11-13 09:19:50 +0000
+++ include/com/ubuntu/connection.h	2013-11-27 08:46:56 +0000
@@ -31,8 +31,7 @@
  */
 class Connection
 {
-public:
-    typedef std::function<void()> Disconnector;
+public:    
     typedef std::function<void(const std::function<void()>&)> Dispatcher;
 
     /**
@@ -58,10 +57,12 @@
      */
     inline void dispatch_via(const Dispatcher& dispatcher)
     {
-        d->dispatcher_installer(dispatcher);
+        if (d->dispatcher_installer)
+            d->dispatcher_installer(dispatcher);
     }
 
 private:
+    typedef std::function<void()> Disconnector;
     typedef std::function<void(const Dispatcher&)> DispatcherInstaller;
 
     template<typename ... Arguments> friend class Signal;
@@ -72,6 +73,11 @@
     {
     }
 
+    inline void reset()
+    {
+        d->reset();
+    }
+
     struct Private
     {
         Private(const Connection::Disconnector& disconnector,
@@ -81,23 +87,40 @@
         {
         }
 
+        inline void reset()
+        {
+            std::lock_guard<std::mutex> lg(guard);
+            reset_locked();
+        }
+
+        inline void reset_locked()
+        {
+            static const Connection::Disconnector empty_disconnector{};
+            static const Connection::DispatcherInstaller empty_dispatcher_installer{};
+
+            disconnector = empty_disconnector;
+            dispatcher_installer = empty_dispatcher_installer;
+        }
+
         inline void disconnect()
         {
             static const Connection::Disconnector empty_disconnector{};
 
             std::lock_guard<std::mutex> lg(guard);
 
-            if (!disconnector)
-                return;
+            if (disconnector)
+                disconnector();
 
-            disconnector();
-            disconnector = empty_disconnector;
+            reset_locked();
         }
 
         std::mutex guard;
         Connection::Disconnector disconnector;
         Connection::DispatcherInstaller dispatcher_installer;
     };
+
+    // The whole class is implicitly shared and we thus forward our complete
+    // shared state to a private structure that is lifetime-managed by a shared_ptr.
     std::shared_ptr<Private> d;
 };
 

=== modified file 'include/com/ubuntu/property.h'
--- include/com/ubuntu/property.h	2013-11-13 09:19:50 +0000
+++ include/com/ubuntu/property.h	2013-11-27 08:46:56 +0000
@@ -33,13 +33,6 @@
 template<typename T>
 class Property
 {
-  private:
-    inline static T& mutable_default_value()
-    {
-        static T default_value = T{};
-        return default_value;
-    }
-
   public:
     /**
      * @brief ValueType refers to the type of the contained value.
@@ -47,28 +40,10 @@
     typedef T ValueType;
 
     /**
-     * @brief Queries the default value set up for a specific type.
-     * @return A non-mutable reference to the default value.
-     */
-    inline static const T& default_value()
-    {
-        return mutable_default_value();
-    }
-
-    /**
-     * @brief Adjusts the default value set up for a specific type.
-     * @post default_value() == new_default_value;
-     */
-    inline static void set_default_value(const T& new_default_value)
-    {
-        mutable_default_value() = new_default_value;
-    }
-
-    /**
      * @brief Property creates a new instance of property and initializes the contained value.
      * @param t The initial value, defaults to Property<T>::default_value().
      */
-    inline explicit Property(const T& t = Property<T>::default_value()) : value{t}
+    inline explicit Property(const T& t = T{}) : value{t}
     {
     }
 

=== modified file 'include/com/ubuntu/signal.h'
--- include/com/ubuntu/signal.h	2013-11-13 09:19:50 +0000
+++ include/com/ubuntu/signal.h	2013-11-27 08:46:56 +0000
@@ -53,6 +53,7 @@
 
         Slot slot;
         Connection::Dispatcher dispatcher;
+        Connection connection;
     };
 
 public:
@@ -63,7 +64,12 @@
     {
     }
 
-    inline ~Signal() = default;
+    inline ~Signal()
+    {
+        std::lock_guard<std::mutex> lg(d->guard);
+        for (auto slot : d->slots)
+            slot.connection.reset();
+    }
 
     // Copy construction, assignment and equality comparison are disabled.
     Signal(const Signal&) = delete;
@@ -81,27 +87,38 @@
      */
     inline Connection connect(const Slot& slot) const
     {
+        // Helpers to initialize an invalid connection.
+        static const Connection::Disconnector empty_disconnector{};
+        static const Connection::DispatcherInstaller empty_dispatcher_installer{};
+
         // The default dispatcher immediately executes the function object
         // provided as argument on whatever thread is currently running.
         static const Connection::Dispatcher default_dispatcher
                 = [](const std::function<void()>& handler) { handler(); };
 
+        Connection conn{empty_disconnector, empty_dispatcher_installer};
+
         std::lock_guard<std::mutex> lg(d->guard);
 
         auto result = d->slots.insert(
                     d->slots.end(),
-                    SlotWrapper{slot, default_dispatcher});
-
-        return Connection(
-                    std::bind(
-                        &Private::disconnect_slot_for_iterator,
-                        d,
-                        result),
-                    std::bind(
-                        &Private::install_dispatcher_for_iterator,
-                        d,
-                        std::placeholders::_1,
-                        result));
+                    SlotWrapper{slot, default_dispatcher, conn});
+
+        // We implicitly share our internal state with the connection here
+        // by passing in our private bits contained in 'd' to the std::bind call.
+        // This admittedly uncommon approach allows us to cleanly manage connection
+        // and signal lifetimes without the need to mark everything as mutable.
+        conn.d->disconnector = std::bind(
+                    &Private::disconnect_slot_for_iterator,
+                    d,
+                    result);
+        conn.d->dispatcher_installer = std::bind(
+                    &Private::install_dispatcher_for_iterator,
+                    d,
+                    std::placeholders::_1,
+                    result);
+
+        return conn;
     }
 
     /**
@@ -148,7 +165,8 @@
 };
 
 /**
- * @brief A signal class that observers can subscribe to, template specialization for signals without arguments.
+ * @brief A signal class that observers can subscribe to,
+ * template specialization for signals without arguments.
  */
 template<>
 class Signal<void>
@@ -169,6 +187,7 @@
 
         Slot slot;
         Connection::Dispatcher dispatcher;
+        Connection connection;
     };
 
 public:
@@ -179,7 +198,12 @@
     {
     }
 
-    inline ~Signal() = default;
+    inline ~Signal()
+    {
+        std::lock_guard<std::mutex> lg(d->guard);
+        for (auto slot : d->slots)
+            slot.connection.reset();
+    }
 
     // Copy construction, assignment and equality comparison are disabled.
     Signal(const Signal&) = delete;
@@ -197,25 +221,38 @@
      */
     inline Connection connect(const Slot& slot) const
     {
+        // Helpers to initialize an invalid connection.
+        static const Connection::Disconnector empty_disconnector{};
+        static const Connection::DispatcherInstaller empty_dispatcher_installer{};
+
         // The default dispatcher immediately executes the function object
         // provided as argument on whatever thread is currently running.
         static const Connection::Dispatcher default_dispatcher
                 = [](const std::function<void()>& handler) { handler(); };
+
+        Connection conn{empty_disconnector, empty_dispatcher_installer};
+
         std::lock_guard<std::mutex> lg(d->guard);
+
         auto result = d->slots.insert(
                     d->slots.end(),
-                    SlotWrapper{slot, default_dispatcher});
-
-        return Connection(
-                    std::bind(
-                        &Private::disconnect_slot_for_iterator,
-                        d,
-                        result),
-                    std::bind(
-                        &Private::install_dispatcher_for_iterator,
-                        d,
-                        std::placeholders::_1,
-                        result));
+                    SlotWrapper{slot, default_dispatcher, conn});
+
+        // We implicitly share our internal state with the connection here
+        // by passing in our private bits contained in 'd' to the std::bind call.
+        // This admittedly uncommon approach allows us to cleanly manage connection
+        // and signal lifetimes without the need to mark everything as mutable.
+        conn.d->disconnector = std::bind(
+                    &Private::disconnect_slot_for_iterator,
+                    d,
+                    result);
+        conn.d->dispatcher_installer = std::bind(
+                    &Private::install_dispatcher_for_iterator,
+                    d,
+                    std::placeholders::_1,
+                    result);
+
+        return conn;
     }
 
     /**

=== modified file 'tests/properties_test.cpp'
--- tests/properties_test.cpp	2013-11-13 10:41:06 +0000
+++ tests/properties_test.cpp	2013-11-27 08:46:56 +0000
@@ -26,16 +26,13 @@
     EXPECT_EQ(int{}, p1.get());
 
     static const int new_default_value = 42;
-    com::ubuntu::Property<int>::set_default_value(new_default_value);
-    com::ubuntu::Property<int> p2;
+    com::ubuntu::Property<int> p2{new_default_value};
 
     EXPECT_EQ(new_default_value, p2.get());
 }
 
 TEST(Property, copy_construction_yields_correct_value)
 {
-    com::ubuntu::Property<int>::set_default_value(0);
-
     static const int default_value = 42;
     com::ubuntu::Property<int> p1{default_value};
     com::ubuntu::Property<int> p2{p1};
@@ -45,8 +42,6 @@
 
 TEST(Property, assignment_operator_for_properties_works)
 {
-    com::ubuntu::Property<int>::set_default_value(0);
-
     static const int default_value = 42;
     com::ubuntu::Property<int> p1{default_value};
     com::ubuntu::Property<int> p2;
@@ -57,8 +52,6 @@
 
 TEST(Property, assignment_operator_for_raw_values_works)
 {
-    com::ubuntu::Property<int>::set_default_value(0);
-
     static const int default_value = 42;
     com::ubuntu::Property<int> p1;
     p1 = default_value;
@@ -68,8 +61,6 @@
 
 TEST(Property, equality_operator_for_properties_works)
 {
-    com::ubuntu::Property<int>::set_default_value(0);
-
     static const int default_value = 42;
     com::ubuntu::Property<int> p1{default_value};
     com::ubuntu::Property<int> p2;
@@ -80,8 +71,6 @@
 
 TEST(Property, equality_operator_for_raw_values_works)
 {
-    com::ubuntu::Property<int>::set_default_value(0);
-
     static const int default_value = 42;
     com::ubuntu::Property<int> p1{default_value};
 
@@ -110,8 +99,6 @@
 
 TEST(Property, signal_changed_is_emitted_with_correct_value_for_set)
 {
-    com::ubuntu::Property<int>::set_default_value(0);
-
     static const int default_value = 42;
     com::ubuntu::Property<int> p1;
     Expectation<int> expectation{default_value};
@@ -125,8 +112,6 @@
 
 TEST(Property, signal_changed_is_emitted_with_correct_value_for_assignment)
 {
-    com::ubuntu::Property<int>::set_default_value(0);
-
     static const int default_value = 42;
     com::ubuntu::Property<int> p1;
 
@@ -141,8 +126,6 @@
 
 TEST(Property, signal_changed_is_emitted_with_correct_value_for_update)
 {
-    com::ubuntu::Property<int>::set_default_value(0);
-
     static const int default_value = 42;
     com::ubuntu::Property<int> p1;
 

=== modified file 'tests/signals_test.cpp'
--- tests/signals_test.cpp	2013-11-13 09:19:50 +0000
+++ tests/signals_test.cpp	2013-11-27 08:46:56 +0000
@@ -93,6 +93,20 @@
     EXPECT_FALSE(expectation.satisfied());
 }
 
+TEST(Signal, a_signal_going_out_of_scope_disconnects_from_slots)
+{
+    auto signal = std::make_shared<com::ubuntu::Signal<int>>();
+
+    auto connection = signal->connect([](int value) { std::cout << value << std::endl; });
+
+    signal.reset();
+
+    com::ubuntu::Connection::Dispatcher dispatcher{};
+
+    EXPECT_NO_THROW(connection.disconnect());
+    EXPECT_NO_THROW(connection.dispatch_via(dispatcher));
+}
+
 #include <queue>
 
 namespace
@@ -116,8 +130,11 @@
                         std::chrono::milliseconds{500},
                         [this]() { return handlers.size() > 0; });
 
-            handlers.front()();
-            handlers.pop();
+            while (handlers.size() > 0)
+            {
+                handlers.front()();
+                handlers.pop();
+            }
         }
     }
 
@@ -150,12 +167,10 @@
     // thread the handler is being called upon equals the thread that the
     // event loop is running upon.
     auto connection = s.connect(
-                [&dispatcher, dispatcher_thread_id](int value, double d)
+                [&dispatcher, dispatcher_thread_id](int value, double)
                 {
                     EXPECT_EQ(dispatcher_thread_id,
-                              std::this_thread::get_id());
-
-                    std::cout << d << std::endl;
+                              std::this_thread::get_id());                    
 
                     if (value == expected_invocation_count)
                         dispatcher.stop();