Merge lp:~michihenning/unity-scopes-api/proxy-fix into lp:unity-scopes-api

[Michi Henning]

Made proxies properly polymorphic, so they derived from a common base and no longer redudantly store
the pimpl and the MWProxy at each level of the hierarchy.

Made proxies properly polymorphic, so they derived from a common base and no longer redudantly store
the pimpl and the MWProxy at each level of the hierarchy.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:67
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-ci/25/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-amd64-ci/25
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-armhf-ci/25
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-armhf-ci/25/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-i386-ci/25

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-ci/25/rebuild

review: Approve (continuous-integration)

[Paweł Stołowski (stolowski)]

LGTM!

review: Approve

=== modified file 'include/scopes/ObjectProxy.h'
--- include/scopes/ObjectProxy.h	2013-11-20 12:33:42 +0000
+++ include/scopes/ObjectProxy.h	2013-11-25 04:24:33 +0000
@@ -36,20 +36,20 @@
 class ObjectProxyImpl;
 }
 
-// TODO: currently incomplete, so there is no true base proxy. The technique outlined in http://tinyrul.com/3w6elg
-// would be suitable to get covariant return for a pimpl() method that returns a shared_ptr. That way, the pimpl()
-// method could be protected here in the base, and we could use ladder inheritance for both the hierarchies
-// rooted at ObjectProxy and ObjectProxyImpl.
-
 class UNITY_API ObjectProxy
 {
 public:
     ObjectProxy();
     virtual ~ObjectProxy() noexcept;
 
+protected:
+    internal::ObjectProxyImpl* pimpl() const noexcept; // Non-virtual because we can't use covariance with incomplete types
+                                                       // Each derived proxy type implements a non-virtual fwd() method
+                                                       // that is called from within each operation to down-cast the pimpl().
+    ObjectProxy(internal::ObjectProxyImpl*);
+    friend class internal::ObjectProxyImpl; // Instantiated only by ObjectProxyImpl
+
 private:
-    friend class internal::ObjectProxyImpl;
-
     std::unique_ptr<internal::ObjectProxyImpl> p;
 };
 

=== modified file 'include/scopes/QueryCtrl.h'
--- include/scopes/QueryCtrl.h	2013-11-20 12:33:42 +0000
+++ include/scopes/QueryCtrl.h	2013-11-25 04:24:33 +0000
@@ -40,7 +40,7 @@
 \brief QueryCtrl allows a query to be cancelled.
 */
 
-class UNITY_API QueryCtrl : public ObjectProxy
+class UNITY_API QueryCtrl : public virtual ObjectProxy
 {
 public:
     /**
@@ -58,13 +58,12 @@
     */
     virtual ~QueryCtrl() noexcept;
 
+protected:
+    QueryCtrl(internal::QueryCtrlImpl* impl);         // Instantiated only by QueryCtrlImpl
+    friend class internal::QueryCtrlImpl;
+
 private:
-    QueryCtrl(internal::QueryCtrlImpl* impl);         // Instantiable only by QueryCtrlImpl
-    // @cond
-    friend class internal::QueryCtrlImpl;
-    // @endcond
-
-    std::unique_ptr<internal::QueryCtrlImpl> p;
+    internal::QueryCtrlImpl* fwd() const;
 };
 
 } // namespace scopes

=== modified file 'include/scopes/Registry.h'
--- include/scopes/Registry.h	2013-11-22 04:00:45 +0000
+++ include/scopes/Registry.h	2013-11-25 04:24:33 +0000
@@ -49,7 +49,7 @@
 You can obtain a proxy to the registry by calling Runtime::registry().
 */
 
-class UNITY_API Registry : public ObjectProxy
+class UNITY_API Registry : public virtual ObjectProxy
 {
 public:
     /// @cond
@@ -67,11 +67,12 @@
     */
     ScopeMap list() const;
 
+protected:
+    Registry(internal::RegistryImpl* impl);          // Instantiated only by RegistryImpl
+    friend class internal::RegistryImpl;
+
 private:
-    Registry(internal::RegistryImpl* impl);          // Instantiable only by RegistryImpl
-    friend class internal::RegistryImpl;
-
-    std::unique_ptr<internal::RegistryImpl> p;
+    internal::RegistryImpl* fwd() const;
 };
 
 } // namespace scopes

=== modified file 'include/scopes/Reply.h'
--- include/scopes/Reply.h	2013-11-22 04:00:45 +0000
+++ include/scopes/Reply.h	2013-11-25 04:24:33 +0000
@@ -44,7 +44,7 @@
 \brief Reply allows the results of a query to be sent to the source of the query.
 */
 
-class UNITY_API Reply : public ObjectProxy
+class UNITY_API Reply : public virtual ObjectProxy
 {
 public:
     Reply(Reply const&) = default;
@@ -96,11 +96,13 @@
     */
     virtual ~Reply() noexcept;
 
+protected:
+    Reply(internal::ReplyImpl* impl);         // Instantiated only by ReplyImpl
+    friend class internal::ReplyImpl;
+
 private:
-    Reply(internal::ReplyImpl* impl);         // Instantiable only by ReplyImpl
-    friend class internal::ReplyImpl;
+    internal::ReplyImpl* fwd() const;
 
-    std::unique_ptr<internal::ReplyImpl> p;
     std::shared_ptr<internal::QueryObject> qo; // Points at the corresponding QueryObject, so we can
                                                // forward cancellation.
 };

=== modified file 'include/scopes/Scope.h'
--- include/scopes/Scope.h	2013-11-21 10:12:45 +0000
+++ include/scopes/Scope.h	2013-11-25 04:24:33 +0000
@@ -43,7 +43,7 @@
 \brief Allows queries to be sent to a scope and results for the query to be retrieved.
 */
 
-class UNITY_API Scope : public ObjectProxy
+class UNITY_API Scope : public virtual ObjectProxy
 {
 public:
     /**
@@ -61,11 +61,12 @@
     */
     virtual ~Scope() noexcept;
 
+protected:
+    Scope(internal::ScopeImpl* impl);          // Instantiated only by ScopeImpl
+    friend class internal::ScopeImpl;
+
 private:
-    Scope(internal::ScopeImpl* impl);          // Instantiable only by ScopeImpl
-    friend class internal::ScopeImpl;
-
-    std::unique_ptr<internal::ScopeImpl> p;
+    internal::ScopeImpl* fwd() const;
 };
 
 } // namespace scopes

=== modified file 'include/scopes/internal/ObjectProxyImpl.h'
--- include/scopes/internal/ObjectProxyImpl.h	2013-11-01 12:44:48 +0000
+++ include/scopes/internal/ObjectProxyImpl.h	2013-11-25 04:24:33 +0000
@@ -34,17 +34,20 @@
 namespace internal
 {
 
-class ObjectProxyImpl final
+class ObjectProxyImpl
 {
 public:
     ObjectProxyImpl(MWProxy const& mw_proxy);
-    ~ObjectProxyImpl() noexcept;
+    virtual ~ObjectProxyImpl() noexcept;
+
+protected:
+    MWProxy proxy() const;                          // Non-virtual because we cannot use covariance with incomplete types.
+                                                    // Each derived proxy implements a non-virtual fwd() method
+                                                    // that is called from within each operation to down-cast the MWProxy.
 
 private:
-
     static Proxy create(MWProxy const& mw_proxy);
 
-private:
     MWProxy mw_proxy_;
 };
 

=== modified file 'include/scopes/internal/QueryCtrlImpl.h'
--- include/scopes/internal/QueryCtrlImpl.h	2013-11-01 12:44:48 +0000
+++ include/scopes/internal/QueryCtrlImpl.h	2013-11-25 04:24:33 +0000
@@ -21,6 +21,7 @@
 
 #include <scopes/internal/MWQueryCtrlProxyFwd.h>
 #include <scopes/internal/MWReplyProxyFwd.h>
+#include <scopes/internal/ObjectProxyImpl.h>
 #include <scopes/QueryCtrlProxyFwd.h>
 
 namespace unity
@@ -41,19 +42,19 @@
 // Calls to push() after finished() was called are ignored.
 // If the proxy goes out of scope before finished was called, it implicitly calls finished().
 
-class QueryCtrlImpl final
+class QueryCtrlImpl : public virtual ObjectProxyImpl
 {
 public:
-    ~QueryCtrlImpl() noexcept;
+    QueryCtrlImpl(MWQueryCtrlProxy const& ctrl_proxy, MWReplyProxy const& reply_proxy);
+    virtual ~QueryCtrlImpl() noexcept;
 
     void cancel();
 
     static QueryCtrlProxy create(MWQueryCtrlProxy const& ctrl_proxy, MWReplyProxy const& reply_proxy);
 
 private:
-    QueryCtrlImpl(MWQueryCtrlProxy const& ctrl_proxy, MWReplyProxy const& reply_proxy);
+    MWQueryCtrlProxy fwd() const;
 
-    MWQueryCtrlProxy ctrl_proxy_;
     MWReplyProxy reply_proxy_;
 };
 

=== modified file 'include/scopes/internal/RegistryImpl.h'
--- include/scopes/internal/RegistryImpl.h	2013-11-18 23:41:30 +0000
+++ include/scopes/internal/RegistryImpl.h	2013-11-25 04:24:33 +0000
@@ -20,6 +20,7 @@
 #define UNITY_INTERNAL_REGISTRYIMPL_H
 
 #include <scopes/internal/MWRegistryProxyFwd.h>
+#include <scopes/internal/ObjectProxyImpl.h>
 #include <scopes/Registry.h>
 
 namespace unity
@@ -36,7 +37,7 @@
 
 class RuntimeImpl;
 
-class RegistryImpl final
+class RegistryImpl : public virtual ObjectProxyImpl
 {
 public:
     RegistryImpl(MWRegistryProxy const& mw_proxy, RuntimeImpl* runtime);
@@ -48,7 +49,7 @@
     static RegistryProxy create(MWRegistryProxy const& mw_proxy, RuntimeImpl* runtime);
 
 private:
-    MWRegistryProxy const mw_proxy_;
+    MWRegistryProxy fwd() const;
 };
 
 } // namespace internal

=== modified file 'include/scopes/internal/ReplyImpl.h'
--- include/scopes/internal/ReplyImpl.h	2013-11-22 04:00:45 +0000
+++ include/scopes/internal/ReplyImpl.h	2013-11-25 04:24:33 +0000
@@ -21,6 +21,7 @@
 
 #include <scopes/internal/MWReplyProxyFwd.h>
 #include <scopes/internal/CategoryRegistry.h>
+#include <scopes/internal/ObjectProxyImpl.h>
 #include <scopes/ReplyProxyFwd.h>
 #include <scopes/Category.h>
 #include <scopes/ReceiverBase.h>
@@ -49,11 +50,11 @@
 // Calls to push() after finished() was called are ignored.
 // If the proxy goes out of scope before finished was called, it implicitly calls finished().
 
-class ReplyImpl final
+class ReplyImpl : public virtual ObjectProxyImpl
 {
 public:
     ReplyImpl(MWReplyProxy const& mw_proxy, std::shared_ptr<QueryObject>const & qo);
-    ~ReplyImpl() noexcept;
+    virtual ~ReplyImpl() noexcept;
 
     Category::SCPtr register_category(std::string const& id, std::string const& title, std::string const &icon, std::string const& renderer_template);
     void register_category(Category::SCPtr category);
@@ -70,8 +71,8 @@
 private:
     bool push(Category::SCPtr category);
     bool push(VariantMap const& variant_map);
+    MWReplyProxy fwd() const;
 
-    MWReplyProxy mw_proxy_;
     std::shared_ptr<QueryObject> qo_;
     std::shared_ptr<CategoryRegistry> cat_registry_;
     std::atomic_bool finished_;

=== modified file 'include/scopes/internal/ScopeImpl.h'
--- include/scopes/internal/ScopeImpl.h	2013-11-21 10:12:45 +0000
+++ include/scopes/internal/ScopeImpl.h	2013-11-25 04:24:33 +0000
@@ -20,6 +20,7 @@
 #define UNITY_INTERNAL_SCOPEIMPL_H
 
 #include <scopes/internal/MWScopeProxyFwd.h>
+#include <scopes/internal/ObjectProxyImpl.h>
 #include <scopes/QueryCtrlProxyFwd.h>
 #include <scopes/ReceiverBase.h>
 #include <scopes/ScopeProxyFwd.h>
@@ -39,18 +40,19 @@
 
 class RuntimeImpl;
 
-class ScopeImpl final
+class ScopeImpl : public virtual ObjectProxyImpl
 {
 public:
     ScopeImpl(MWScopeProxy const& mw_proxy, RuntimeImpl* runtime);
-    ~ScopeImpl() noexcept;
+    virtual ~ScopeImpl() noexcept;
 
     QueryCtrlProxy create_query(std::string const& q, VariantMap const& hints, ReceiverBase::SPtr const& reply) const;
 
     static ScopeProxy create(MWScopeProxy const& mw_proxy, RuntimeImpl* runtime);
 
 private:
-    MWScopeProxy mw_proxy_;
+    MWScopeProxy fwd() const;
+
     RuntimeImpl* const runtime_;
 };
 

=== modified file 'src/ObjectProxy.cpp'
--- src/ObjectProxy.cpp	2013-11-01 12:44:48 +0000
+++ src/ObjectProxy.cpp	2013-11-25 04:24:33 +0000
@@ -42,10 +42,20 @@
 {
 }
 
+ObjectProxy::ObjectProxy(internal::ObjectProxyImpl* pimpl) :
+    p(pimpl)
+{
+}
+
 ObjectProxy::~ObjectProxy() noexcept
 {
 }
 
+internal::ObjectProxyImpl* ObjectProxy::pimpl() const noexcept
+{
+    return p.get();
+}
+
 //! @endcond
 
 } // namespace scopes

=== modified file 'src/QueryCtrl.cpp'
--- src/QueryCtrl.cpp	2013-11-01 12:44:48 +0000
+++ src/QueryCtrl.cpp	2013-11-25 04:24:33 +0000
@@ -32,7 +32,7 @@
 //! @cond
 
 QueryCtrl::QueryCtrl(internal::QueryCtrlImpl* impl)
-    : p(impl)
+    : ObjectProxy(impl)
 {
 }
 
@@ -42,7 +42,12 @@
 
 void QueryCtrl::cancel() const
 {
-    return p->cancel();
+    return fwd()->cancel();
+}
+
+internal::QueryCtrlImpl* QueryCtrl::fwd() const
+{
+    return dynamic_cast<internal::QueryCtrlImpl*>(pimpl());
 }
 
 //! @endcond

=== modified file 'src/Registry.cpp'
--- src/Registry.cpp	2013-11-01 12:44:48 +0000
+++ src/Registry.cpp	2013-11-25 04:24:33 +0000
@@ -32,7 +32,7 @@
 //! @cond
 
 Registry::Registry(internal::RegistryImpl* impl) :
-    p(impl)
+    ObjectProxy(impl)
 {
 }
 
@@ -44,12 +44,17 @@
 
 ScopeProxy Registry::find(std::string const& scope_name) const
 {
-    return p->find(scope_name);
+    return fwd()->find(scope_name);
 }
 
 ScopeMap Registry::list() const
 {
-    return p->list();
+    return fwd()->list();
+}
+
+internal::RegistryImpl* Registry::fwd() const
+{
+    return dynamic_cast<internal::RegistryImpl*>(pimpl());
 }
 
 } // namespace scopes

=== modified file 'src/Reply.cpp'
--- src/Reply.cpp	2013-11-18 14:58:07 +0000
+++ src/Reply.cpp	2013-11-25 04:24:33 +0000
@@ -32,8 +32,8 @@
 
 //! @cond
 
-Reply::Reply(internal::ReplyImpl* impl)
-    : p(impl)
+Reply::Reply(internal::ReplyImpl* impl) :
+    ObjectProxy(impl)
 {
 }
 
@@ -43,27 +43,32 @@
 
 Category::SCPtr Reply::register_category(std::string const& id, std::string const& title, std::string const &icon, std::string const& renderer_template)
 {
-    return p->register_category(id, title, icon, renderer_template);
+    return fwd()->register_category(id, title, icon, renderer_template);
 }
 
 void Reply::register_category(Category::SCPtr category)
 {
-    return p->register_category(category);
+    return fwd()->register_category(category);
 }
 
 Category::SCPtr Reply::lookup_category(std::string const& id) const
 {
-    return p->lookup_category(id);
+    return fwd()->lookup_category(id);
 }
 
 bool Reply::push(ResultItem const& result) const
 {
-    return p->push(result);
+    return fwd()->push(result);
 }
 
 void Reply::finished() const
 {
-    return p->finished();
+    return fwd()->finished();
+}
+
+internal::ReplyImpl* Reply::fwd() const
+{
+    return dynamic_cast<internal::ReplyImpl*>(pimpl());
 }
 
 //! @endcond

=== modified file 'src/Scope.cpp'
--- src/Scope.cpp	2013-11-21 10:12:45 +0000
+++ src/Scope.cpp	2013-11-25 04:24:33 +0000
@@ -32,7 +32,7 @@
 //! @cond
 
 Scope::Scope(internal::ScopeImpl* impl) :
-    p(impl)
+    ObjectProxy(impl)
 {
 }
 
@@ -42,7 +42,12 @@
 
 QueryCtrlProxy Scope::create_query(std::string const& q, VariantMap const& hints, ReceiverBase::SPtr const& reply) const
 {
-    return p->create_query(q, hints, reply);
+    return fwd()->create_query(q, hints, reply);
+}
+
+internal::ScopeImpl* Scope::fwd() const
+{
+    return dynamic_cast<internal::ScopeImpl*>(pimpl());
 }
 
 //! @endcond

=== modified file 'src/internal/ObjectProxyImpl.cpp'
--- src/internal/ObjectProxyImpl.cpp	2013-11-18 23:53:56 +0000
+++ src/internal/ObjectProxyImpl.cpp	2013-11-25 04:24:33 +0000
@@ -49,6 +49,11 @@
     return make_shared<ObjectProxy>();
 }
 
+MWProxy ObjectProxyImpl::proxy() const
+{
+    return mw_proxy_;
+}
+
 } // namespace internal
 
 } // namespace scopes

=== modified file 'src/internal/QueryCtrlImpl.cpp'
--- src/internal/QueryCtrlImpl.cpp	2013-11-22 04:00:45 +0000
+++ src/internal/QueryCtrlImpl.cpp	2013-11-25 04:24:33 +0000
@@ -42,7 +42,7 @@
 {
 
 QueryCtrlImpl::QueryCtrlImpl(MWQueryCtrlProxy const& ctrl_proxy, MWReplyProxy const& reply_proxy) :
-    ctrl_proxy_(ctrl_proxy),
+    ObjectProxyImpl(ctrl_proxy),
     reply_proxy_(reply_proxy)
 {
     assert(ctrl_proxy);
@@ -60,7 +60,7 @@
     try
     {
         // Forward cancellation down-stream to the query. This does not block.
-        ctrl_proxy_->cancel();
+        fwd()->cancel();
 
         // Indicate (to ourselves) that this query is complete. Calling via the MWReplyProxy ensures
         // the finished() call will be processed by a seperate server-side thread,
@@ -78,6 +78,11 @@
     return QueryCtrlProxy(new QueryCtrl(new QueryCtrlImpl(ctrl_proxy, reply_proxy)));
 }
 
+MWQueryCtrlProxy QueryCtrlImpl::fwd() const
+{
+    return dynamic_pointer_cast<MWQueryCtrl>(proxy());
+}
+
 } // namespace internal
 
 } // namespace scopes

=== modified file 'src/internal/RegistryImpl.cpp'
--- src/internal/RegistryImpl.cpp	2013-11-18 23:41:30 +0000
+++ src/internal/RegistryImpl.cpp	2013-11-25 04:24:33 +0000
@@ -37,7 +37,7 @@
 {
 
 RegistryImpl::RegistryImpl(MWRegistryProxy const& mw_proxy, RuntimeImpl*) :
-    mw_proxy_(mw_proxy)
+    ObjectProxyImpl(mw_proxy)
 {
 }
 
@@ -47,12 +47,12 @@
 
 ScopeProxy RegistryImpl::find(std::string const& scope_name)
 {
-    return mw_proxy_->find(scope_name);
+    return fwd()->find(scope_name);
 }
 
 ScopeMap RegistryImpl::list()
 {
-    return mw_proxy_->list();
+    return fwd()->list();
 }
 
 RegistryProxy RegistryImpl::create(MWRegistryProxy const& mw_proxy, RuntimeImpl* runtime)
@@ -60,6 +60,11 @@
     return RegistryProxy(new Registry(new RegistryImpl(mw_proxy, runtime)));
 }
 
+MWRegistryProxy RegistryImpl::fwd() const
+{
+    return dynamic_pointer_cast<MWRegistry>(proxy());
+}
+
 } // namespace internal
 
 } // namespace scopes

=== modified file 'src/internal/ReplyImpl.cpp'
--- src/internal/ReplyImpl.cpp	2013-11-22 04:00:45 +0000
+++ src/internal/ReplyImpl.cpp	2013-11-25 04:24:33 +0000
@@ -45,7 +45,7 @@
 {
 
 ReplyImpl::ReplyImpl(MWReplyProxy const& mw_proxy, std::shared_ptr<QueryObject> const& qo) :
-    mw_proxy_(mw_proxy),
+    ObjectProxyImpl(mw_proxy),
     qo_(qo),
     cat_registry_(new CategoryRegistry()),
     finished_(false)
@@ -125,7 +125,7 @@
     {
         try
         {
-            mw_proxy_->push(variant_map);
+            fwd()->push(variant_map);
             return true;
         }
         catch (MiddlewareException const& e)
@@ -149,7 +149,7 @@
     {
         try
         {
-            mw_proxy_->finished(reason);
+            fwd()->finished(reason);
         }
         catch (MiddlewareException const& e)
         {
@@ -160,7 +160,12 @@
 
 ReplyProxy ReplyImpl::create(MWReplyProxy const& mw_proxy, std::shared_ptr<QueryObject> const& qo)
 {
-    return ReplyProxy(new Reply(new ReplyImpl(mw_proxy, qo)));
+    return ReplyProxy(new Reply((new ReplyImpl(mw_proxy, qo))));
+}
+
+MWReplyProxy ReplyImpl::fwd() const
+{
+    return dynamic_pointer_cast<MWReply>(proxy());
 }
 
 } // namespace internal

=== modified file 'src/internal/ScopeImpl.cpp'
--- src/internal/ScopeImpl.cpp	2013-11-22 04:00:45 +0000
+++ src/internal/ScopeImpl.cpp	2013-11-25 04:24:33 +0000
@@ -40,7 +40,7 @@
 {
 
 ScopeImpl::ScopeImpl(MWScopeProxy const& mw_proxy, RuntimeImpl* runtime) :
-    mw_proxy_(mw_proxy),
+    ObjectProxyImpl(mw_proxy),
     runtime_(runtime)
 {
     assert(runtime);
@@ -58,7 +58,7 @@
         // Create a middleware server-side object that can receive incoming
         // push() and finished() messages over the network.
         ReplyObject::SPtr ro(make_shared<ReplyObject>(reply, runtime_));
-        MWReplyProxy rp = mw_proxy_->mw_base()->add_reply_object(ro);
+        MWReplyProxy rp = fwd()->mw_base()->add_reply_object(ro);
 
         // Forward the the create_query() method across the bus. This is a
         // synchronous twoway interaction with the scope, so it can return
@@ -66,7 +66,7 @@
         // thread for create_query() calls, this is guaranteed not to block for
         // any length of time. (No application code other than the QueryBase constructor
         // is called by create_query() on the server side.)
-        ctrl = mw_proxy_->create_query(q, hints, rp);
+        ctrl = fwd()->create_query(q, hints, rp);
         assert(ctrl);
     }
     catch (unity::Exception const& e)
@@ -92,6 +92,11 @@
     return ScopeProxy(new Scope(new ScopeImpl(mw_proxy, runtime)));
 }
 
+MWScopeProxy ScopeImpl::fwd() const
+{
+    return dynamic_pointer_cast<MWScope>(proxy());
+}
+
 } // namespace internal
 
 } // namespace scopes