Redesign inheritance from Message (#62)

... so that the code is more idiomatic, clear and expressive about its intended use

docs/using-sdbus-c++.md

@@ -132,11 +132,15 @@ The following diagram illustrates the major entities in sdbus-c++.
   * invoking remote methods of the corresponding object, in both synchronous and asynchronous way,
   * registering handlers for signals,
 
-`Message` class represents a message, which is the fundamental DBus concept. There are three distinctive types of message that derive from the `Message` class:
+`Message` class represents a message, which is the fundamental DBus concept. There are three distinctive types of message that are derived from the `Message` class:
 
   * `MethodCall` (with serialized parameters),
+  * `AsyncMethodCall` (with serialized parameters),
   * `MethodReply` (with serialized return values),
-  * or a `Signal` (with serialized parameters).
+  * `Signal` (with serialized parameters),
+  * `PropertySetCall` (with serialized parameter value to be set)
+  * `PropertyGetReply` (where property value shall be stored)
+  * `PlainMessage` (for internal purposes).
 
 ### Thread safety in sdbus-c++
 

include/sdbus-c++/ConvenienceApiClasses.inl

@@ -253,10 +253,10 @@ namespace sdbus {
         if (propertySignature_.empty())
             propertySignature_ = signature_of_function_output_arguments<_Function>::str();
 
-        getter_ = [callback = std::forward<_Function>(callback)](Message& msg)
+        getter_ = [callback = std::forward<_Function>(callback)](PropertyGetReply& reply)
         {
-            // Get the propety value and serialize it into the message
+            // Get the propety value and serialize it into the pre-constructed reply message
-            msg << callback();
+            reply << callback();
         };
 
         return *this;
@@ -271,14 +271,14 @@ namespace sdbus {
         if (propertySignature_.empty())
             propertySignature_ = signature_of_function_input_arguments<_Function>::str();
 
-        setter_ = [callback = std::forward<_Function>(callback)](Message& msg)
+        setter_ = [callback = std::forward<_Function>(callback)](PropertySetCall& call)
         {
             // Default-construct property value
             using property_type = function_argument_t<_Function, 0>;
             std::decay_t<property_type> property;
 
-            // Deserialize property value from the message
+            // Deserialize property value from the incoming call message
-            msg >> property;
+            call >> property;
 
             // Invoke setter with the value
             callback(property);

include/sdbus-c++/Message.h

@@ -77,16 +77,6 @@ namespace sdbus {
     class Message
     {
     public:
-        Message() = default;
-        Message(internal::ISdBus* sdbus) noexcept;
-        Message(void *msg, internal::ISdBus* sdbus) noexcept;
-        Message(void *msg, internal::ISdBus* sdbus, adopt_message_t) noexcept;
-        Message(const Message&) noexcept;
-        Message& operator=(const Message&) noexcept;
-        Message(Message&& other) noexcept;
-        Message& operator=(Message&& other) noexcept;
-        ~Message();
-
         Message& operator<<(bool item);
         Message& operator<<(int16_t item);
         Message& operator<<(int32_t item);
@@ -150,6 +140,23 @@ namespace sdbus {
         void seal();
         void rewind(bool complete);
 
+        class Factory;
+
+    protected:
+        Message() = default;
+        Message(internal::ISdBus* sdbus) noexcept;
+        Message(void *msg, internal::ISdBus* sdbus) noexcept;
+        Message(void *msg, internal::ISdBus* sdbus, adopt_message_t) noexcept;
+
+        Message(const Message&) noexcept;
+        Message& operator=(const Message&) noexcept;
+        Message(Message&& other) noexcept;
+        Message& operator=(Message&& other) noexcept;
+
+        ~Message();
+
+        friend Factory;
+
     protected:
         void* msg_{};
         internal::ISdBus* sdbus_{};
@@ -158,8 +165,10 @@ namespace sdbus {
 
     class MethodCall : public Message
     {
-    public:
         using Message::Message;
+
+    public:
+        MethodCall() = default;
         MethodReply send() const;
         MethodReply createReply() const;
         MethodReply createErrorReply(const sdbus::Error& error) const;
@@ -173,29 +182,58 @@ namespace sdbus {
 
     class AsyncMethodCall : public Message
     {
+        using Message::Message;
+
     public:
         using Slot = std::unique_ptr<void, std::function<void(void*)>>;
 
-        using Message::Message;
+        AsyncMethodCall() = default;
-        AsyncMethodCall() = default; // Fixes gcc 6.3 error (default c-tor is not imported in above using declaration)
         AsyncMethodCall(MethodCall&& call) noexcept;
         Slot send(void* callback, void* userData) const;
     };
 
     class MethodReply : public Message
     {
-    public:
         using Message::Message;
+
+    public:
+        MethodReply() = default;
         void send() const;
     };
 
     class Signal : public Message
     {
-    public:
         using Message::Message;
+
+    public:
+        Signal() = default;
         void send() const;
     };
 
+    class PropertySetCall : public Message
+    {
+        using Message::Message;
+
+    public:
+        PropertySetCall() = default;
+    };
+
+    class PropertyGetReply : public Message
+    {
+        using Message::Message;
+
+    public:
+        PropertyGetReply() = default;
+    };
+
+    class PlainMessage : public Message
+    {
+        using Message::Message;
+
+    public:
+        PlainMessage() = default;
+    };
+
     template <typename _Element>
     inline Message& operator<<(Message& msg, const std::vector<_Element>& items)
     {

include/sdbus-c++/TypeTraits.h

@@ -41,10 +41,11 @@ namespace sdbus {
     class ObjectPath;
     class Signature;
     struct UnixFd;
-    class Message;
     class MethodCall;
     class MethodReply;
     class Signal;
+    class PropertySetCall;
+    class PropertyGetReply;
     template <typename... _Results> class Result;
     class Error;
 }
@@ -54,8 +55,8 @@ namespace sdbus {
     using method_callback = std::function<void(MethodCall msg)>;
     using async_reply_handler = std::function<void(MethodReply& reply, const Error* error)>;
     using signal_handler = std::function<void(Signal& signal)>;
-    using property_set_callback = std::function<void(Message& msg)>;
+    using property_set_callback = std::function<void(PropertySetCall& msg)>;
-    using property_get_callback = std::function<void(Message& reply)>;
+    using property_get_callback = std::function<void(PropertyGetReply& reply)>;
 
     template <typename _T>
     struct signature_of

include/sdbus-c++/Types.h

@@ -94,7 +94,7 @@ namespace sdbus {
         std::string peekValueType() const;
 
     private:
-        mutable Message msg_{};
+        mutable PlainMessage msg_{};
     };
 
     /********************************************//**

src/Connection.cpp

@@ -25,6 +25,7 @@
 
 #include "Connection.h"
 #include "SdBus.h"
+#include "MessageUtils.h"
 #include <sdbus-c++/Message.h>
 #include <sdbus-c++/Error.h>
 #include "ScopeGuard.h"
@@ -169,7 +170,7 @@ MethodCall Connection::createMethodCall( const std::string& destination
 
     SDBUS_THROW_ERROR_IF(r < 0, "Failed to create method call", -r);
 
-    return MethodCall{sdbusMsg, iface_.get(), adopt_message};
+    return Message::Factory::create<MethodCall>(sdbusMsg, iface_.get(), adopt_message);
 }
 
 Signal Connection::createSignal( const std::string& objectPath
@@ -186,7 +187,7 @@ Signal Connection::createSignal( const std::string& objectPath
 
     SDBUS_THROW_ERROR_IF(r < 0, "Failed to create signal", -r);
 
-    return Signal{sdbusSignal, iface_.get(), adopt_message};
+    return Message::Factory::create<Signal>(sdbusSignal, iface_.get(), adopt_message);
 }
 
 void Connection::emitPropertiesChangedSignal( const std::string& objectPath

src/Message.cpp

@@ -639,7 +639,7 @@ MethodReply MethodCall::sendWithReply() const
 
     SDBUS_THROW_ERROR_IF(r < 0, "Failed to call method", -r);
 
-    return MethodReply{sdbusReply, sdbus_, adopt_message};
+    return Factory::create<MethodReply>(sdbusReply, sdbus_, adopt_message);
 }
 
 MethodReply MethodCall::sendWithNoReply() const
@@ -647,7 +647,7 @@ MethodReply MethodCall::sendWithNoReply() const
     auto r = sdbus_->sd_bus_send(nullptr, (sd_bus_message*)msg_, nullptr);
     SDBUS_THROW_ERROR_IF(r < 0, "Failed to call method with no reply", -r);
 
-    return MethodReply{}; // No reply
+    return Factory::create<MethodReply>(); // No reply
 }
 
 MethodReply MethodCall::createReply() const
@@ -656,7 +656,7 @@ MethodReply MethodCall::createReply() const
     auto r = sdbus_->sd_bus_message_new_method_return((sd_bus_message*)msg_, &sdbusReply);
     SDBUS_THROW_ERROR_IF(r < 0, "Failed to create method reply", -r);
 
-    return MethodReply{sdbusReply, sdbus_, adopt_message};
+    return Factory::create<MethodReply>(sdbusReply, sdbus_, adopt_message);
 }
 
 MethodReply MethodCall::createErrorReply(const Error& error) const
@@ -669,7 +669,7 @@ MethodReply MethodCall::createErrorReply(const Error& error) const
     auto r = sdbus_->sd_bus_message_new_method_error((sd_bus_message*)msg_, &sdbusErrorReply, &sdbusError);
     SDBUS_THROW_ERROR_IF(r < 0, "Failed to create method error reply", -r);
 
-    return MethodReply{sdbusErrorReply, sdbus_, adopt_message};
+    return Factory::create<MethodReply>(sdbusErrorReply, sdbus_, adopt_message);
 }
 
 AsyncMethodCall::AsyncMethodCall(MethodCall&& call) noexcept
@@ -699,7 +699,7 @@ void Signal::send() const
     SDBUS_THROW_ERROR_IF(r < 0, "Failed to emit signal", -r);
 }
 
-Message createPlainMessage()
+PlainMessage createPlainMessage()
 {
     int r;
 
@@ -736,7 +736,7 @@ Message createPlainMessage()
     r = sd_bus_message_new(bus, &sdbusMsg, _SD_BUS_MESSAGE_TYPE_INVALID);
     SDBUS_THROW_ERROR_IF(r < 0, "Failed to create a new message", -r);
 
-    return Message{sdbusMsg, &sdbus, adopt_message};
+    return Message::Factory::create<PlainMessage>(sdbusMsg, &sdbus, adopt_message);
 }
 
 }

src/MessageUtils.h

@@ -30,7 +30,35 @@
 
 namespace sdbus
 {
-    Message createPlainMessage();
+    class Message::Factory
+    {
+    public:
+        template<typename _Msg>
+        static _Msg create()
+        {
+            return _Msg{};
+        }
+
+        template<typename _Msg>
+        static _Msg create(void *msg)
+        {
+            return _Msg{msg};
+        }
+
+        template<typename _Msg>
+        static _Msg create(void *msg, internal::ISdBus* sdbus)
+        {
+            return _Msg{msg, sdbus};
+        }
+
+        template<typename _Msg>
+        static _Msg create(void *msg, internal::ISdBus* sdbus, adopt_message_t)
+        {
+            return _Msg{msg, sdbus, adopt_message};
+        }
+    };
+
+    PlainMessage createPlainMessage();
 }
 
 #endif /* SDBUS_CXX_INTERNAL_MESSAGEUTILS_H_ */

src/Object.cpp

@@ -24,6 +24,7 @@
  */
 
 #include "Object.h"
+#include "MessageUtils.h"
 #include <sdbus-c++/IConnection.h>
 #include <sdbus-c++/Message.h>
 #include <sdbus-c++/Error.h>
@@ -260,7 +261,7 @@ int Object::sdbus_method_callback(sd_bus_message *sdbusMessage, void *userData,
     auto* object = static_cast<Object*>(userData);
     assert(object != nullptr);
 
-    MethodCall message{sdbusMessage, &object->connection_.getSdBusInterface()};
+    auto message = Message::Factory::create<MethodCall>(sdbusMessage, &object->connection_.getSdBusInterface());
 
     // Note: The lookup can be optimized by using sorted vectors instead of associative containers
     auto& callback = object->interfaces_[message.getInterfaceName()].methods_[message.getMemberName()].callback_;
@@ -298,7 +299,7 @@ int Object::sdbus_property_get_callback( sd_bus */*bus*/
         return 1;
     }
 
-    Message reply{sdbusReply, &object->connection_.getSdBusInterface()};
+    auto reply = Message::Factory::create<PropertyGetReply>(sdbusReply, &object->connection_.getSdBusInterface());
 
     try
     {
@@ -327,7 +328,7 @@ int Object::sdbus_property_set_callback( sd_bus */*bus*/
     auto& callback = object->interfaces_[interface].properties_[property].setCallback_;
     assert(callback);
 
-    Message value{sdbusValue, &object->connection_.getSdBusInterface()};
+    auto value = Message::Factory::create<PropertySetCall>(sdbusValue, &object->connection_.getSdBusInterface());
 
     try
     {

src/Proxy.cpp

@@ -25,6 +25,7 @@
 
 #include "Proxy.h"
 #include "IConnection.h"
+#include "MessageUtils.h"
 #include "sdbus-c++/Message.h"
 #include "sdbus-c++/IConnection.h"
 #include "sdbus-c++/Error.h"
@@ -137,7 +138,7 @@ int Proxy::sdbus_async_reply_handler(sd_bus_message *sdbusMessage, void *userDat
 
     SCOPE_EXIT{ proxy.pendingAsyncCalls_.removeCall(asyncCallData->slot.get()); };
 
-    MethodReply message{sdbusMessage, &proxy.connection_->getSdBusInterface()};
+    auto message = Message::Factory::create<MethodReply>(sdbusMessage, &proxy.connection_->getSdBusInterface());
 
     const auto* error = sd_bus_message_get_error(sdbusMessage);
     if (error == nullptr)
@@ -158,7 +159,7 @@ int Proxy::sdbus_signal_callback(sd_bus_message *sdbusMessage, void *userData, s
     auto* proxy = static_cast<Proxy*>(userData);
     assert(proxy != nullptr);
 
-    Signal message{sdbusMessage, &proxy->connection_->getSdBusInterface()};
+    auto message = Message::Factory::create<Signal>(sdbusMessage, &proxy->connection_->getSdBusInterface());
 
     // Note: The lookup can be optimized by using sorted vectors instead of associative containers
     auto& callback = proxy->interfaces_[message.getInterfaceName()].signals_[message.getMemberName()].callback_;

tests/unittests/Message_test.cpp

@@ -36,7 +36,7 @@ using namespace std::string_literals;
 
 namespace
 {
-    std::string deserializeString(sdbus::Message& msg)
+    std::string deserializeString(sdbus::PlainMessage& msg)
     {
         std::string str;
         msg >> str;
@@ -50,29 +50,29 @@ namespace
 
 TEST(AMessage, CanBeDefaultConstructed)
 {
-    ASSERT_NO_THROW(sdbus::Message());
+    ASSERT_NO_THROW(sdbus::PlainMessage());
 }
 
 TEST(AMessage, IsInvalidAfterDefaultConstructed)
 {
-    sdbus::Message msg;
+    sdbus::PlainMessage msg;
 
     ASSERT_FALSE(msg.isValid());
 }
 
 TEST(AMessage, IsValidWhenConstructedAsRealMessage)
 {
-    sdbus::Message msg{sdbus::createPlainMessage()};
+    auto msg = sdbus::createPlainMessage();
     ASSERT_TRUE(msg.isValid());
 }
 
 TEST(AMessage, CreatesShallowCopyWhenCopyConstructed)
 {
-    sdbus::Message msg{sdbus::createPlainMessage()};
+    auto msg = sdbus::createPlainMessage();
     msg << "I am a string"s;
     msg.seal();
 
-    sdbus::Message msgCopy = msg;
+    sdbus::PlainMessage msgCopy = msg;
 
     std::string str;
     msgCopy >> str;
@@ -83,11 +83,11 @@ TEST(AMessage, CreatesShallowCopyWhenCopyConstructed)
 
 TEST(AMessage, CreatesDeepCopyWhenEplicitlyCopied)
 {
-    sdbus::Message msg{sdbus::createPlainMessage()};
+    auto msg = sdbus::createPlainMessage();
     msg << "I am a string"s;
     msg.seal();
 
-    sdbus::Message msgCopy{sdbus::createPlainMessage()};
+    auto msgCopy = sdbus::createPlainMessage();
     msg.copyTo(msgCopy, true);
     msgCopy.seal(); // Seal to be able to read from it subsequently
     msg.rewind(true); // Rewind to the beginning after copying
@@ -98,14 +98,14 @@ TEST(AMessage, CreatesDeepCopyWhenEplicitlyCopied)
 
 TEST(AMessage, IsEmptyWhenContainsNoValue)
 {
-    sdbus::Message msg{sdbus::createPlainMessage()};
+    auto msg = sdbus::createPlainMessage();
 
     ASSERT_TRUE(msg.isEmpty());
 }
 
 TEST(AMessage, IsNotEmptyWhenContainsAValue)
 {
-    sdbus::Message msg{sdbus::createPlainMessage()};
+    auto msg = sdbus::createPlainMessage();
     msg << "I am a string"s;
 
     ASSERT_FALSE(msg.isEmpty());
@@ -113,7 +113,7 @@ TEST(AMessage, IsNotEmptyWhenContainsAValue)
 
 TEST(AMessage, CanCarryASimpleInteger)
 {
-    sdbus::Message msg{sdbus::createPlainMessage()};
+    auto msg = sdbus::createPlainMessage();
 
     int dataWritten = 5;
 
@@ -128,7 +128,7 @@ TEST(AMessage, CanCarryASimpleInteger)
 
 TEST(AMessage, CanCarryAUnixFd)
 {
-    sdbus::Message msg{sdbus::createPlainMessage()};
+    auto msg = sdbus::createPlainMessage();
 
     sdbus::UnixFd dataWritten = 0;
     msg << dataWritten;
@@ -143,7 +143,7 @@ TEST(AMessage, CanCarryAUnixFd)
 
 TEST(AMessage, CanCarryAVariant)
 {
-    sdbus::Message msg{sdbus::createPlainMessage()};
+    auto msg = sdbus::createPlainMessage();
 
     auto dataWritten = sdbus::Variant((double)3.14);
 
@@ -158,7 +158,7 @@ TEST(AMessage, CanCarryAVariant)
 
 TEST(AMessage, CanCarryACollectionOfEmbeddedVariants)
 {
-    sdbus::Message msg{sdbus::createPlainMessage()};
+    auto msg = sdbus::createPlainMessage();
 
     auto value = std::vector<sdbus::Variant>{"hello"s, (double)3.14};
     auto dataWritten = sdbus::Variant{value};
@@ -175,7 +175,7 @@ TEST(AMessage, CanCarryACollectionOfEmbeddedVariants)
 
 TEST(AMessage, CanCarryAnArray)
 {
-    sdbus::Message msg{sdbus::createPlainMessage()};
+    auto msg = sdbus::createPlainMessage();
 
     std::vector<int64_t> dataWritten{3545342, 43643532, 324325};
 
@@ -190,7 +190,7 @@ TEST(AMessage, CanCarryAnArray)
 
 TEST(AMessage, CanCarryADictionary)
 {
-    sdbus::Message msg{sdbus::createPlainMessage()};
+    auto msg = sdbus::createPlainMessage();
 
     std::map<int, std::string> dataWritten{{1, "one"}, {2, "two"}};
 
@@ -205,7 +205,7 @@ TEST(AMessage, CanCarryADictionary)
 
 TEST(AMessage, CanCarryAComplexType)
 {
-    sdbus::Message msg{sdbus::createPlainMessage()};
+    auto msg = sdbus::createPlainMessage();
 
     using ComplexType = std::map<
                             uint64_t,

tests/unittests/Types_test.cpp

@@ -152,7 +152,7 @@ TEST(ANonEmptyVariant, SerializesSuccessfullyToAMessage)
 {
     sdbus::Variant variant("a string");
 
-    sdbus::Message msg = sdbus::createPlainMessage();
+    auto msg = sdbus::createPlainMessage();
 
     ASSERT_NO_THROW(variant.serializeTo(msg));
 }
@@ -161,7 +161,7 @@ TEST(AnEmptyVariant, ThrowsWhenBeingSerializedToAMessage)
 {
     sdbus::Variant variant;
 
-    sdbus::Message msg = sdbus::createPlainMessage();
+    auto msg = sdbus::createPlainMessage();
 
     ASSERT_THROW(variant.serializeTo(msg), sdbus::Error);
 }
@@ -172,7 +172,7 @@ TEST(ANonEmptyVariant, SerializesToAndDeserializesFromAMessageSuccessfully)
     ComplexType value{ {ANY_UINT64, ComplexType::mapped_type{sdbus::make_struct("hello", ANY_DOUBLE), sdbus::make_struct("world", ANY_DOUBLE)}} };
     sdbus::Variant variant(value);
 
-    sdbus::Message msg = sdbus::createPlainMessage();
+    auto msg = sdbus::createPlainMessage();
     variant.serializeTo(msg);
     msg.seal();
     sdbus::Variant variant2;
@@ -189,7 +189,7 @@ TEST(CopiesOfVariant, SerializeToAndDeserializeFromMessageSuccessfully)
     auto variantCopy1{variant};
     auto variantCopy2 = variant;
 
-    sdbus::Message msg = sdbus::createPlainMessage();
+    auto msg = sdbus::createPlainMessage();
     variant.serializeTo(msg);
     variantCopy1.serializeTo(msg);
     variantCopy2.serializeTo(msg);