refactor: improve and make more efficient some Message API (#432)

docs/using-sdbus-c++.md

@@ -1780,6 +1780,8 @@ sdbus-c++ v2 is a major release that comes with a number of breaking API/ABI/beh
 * `createDefaultBusConnection()` has been renamed to `createBusConnection()`.
 * Change in behavior: `Proxy`s now by default call `createBusConnection()` to get a connection when the connection is not provided explicitly by the caller, so they connect to either the session bus or the system bus depending on the context (as opposed to always to the system bus like before).
 * Callbacks taking `const sdbus::Error* error` were changed to take `std::optional<sdbus::Error>`, which better expresses the intent and meaning.
+* `getInterfaceName()`, `getMemberName()`, `getSender()`, `getPath()` and `getDestination()` methods of `Message` class now return `const char*` instead of `std::string`, for efficiency reasons.
+* `peekType()` method of `Message` class now returns a pair of `char` (type signature) and `const char*` (contents signature), for expressiveness and efficiency reasons.
 * D-Bus signatures when using high-level API are now assembled at compile time. There are breaking changes inside `signature_of` type traits and `Message` serialization/deserialization methods. This only interests you if you extend sdbus-c++ type system with your own types. See the updated tutorial on extending sdbus-c++ type system. 
 * Types and methods marked deprecated in sdbus-c++ v1 were removed completely.
 * CMake options got `SDBUSCPP_` prefix for better usability and minimal risk of conflicts in downstream CMake projects. `SDBUSCPP_INSTALL` CMake option was added.

include/sdbus-c++/Message.h

@@ -34,6 +34,7 @@
 #include <array>
 #include <cassert>
 #include <cstdint>
+#include <cstring>
 #include <functional>
 #include <map>
 #ifdef __has_include
@@ -52,14 +53,10 @@
 namespace sdbus {
     class Variant;
     class ObjectPath;
-    class InterfaceName;
-    class MemberName;
     class Signature;
     template <typename... _ValueTypes> class Struct;
     class UnixFd;
     class MethodReply;
-    class BusName;
-    using ConnectionName = BusName;
     namespace internal {
         class ISdBus;
     }
@@ -202,12 +199,13 @@ namespace sdbus {
         explicit operator bool() const;
         void clearFlags();
 
-        InterfaceName getInterfaceName() const;
+        const char* getInterfaceName() const;
-        MemberName getMemberName() const;
+        const char* getMemberName() const;
-        ConnectionName getSender() const;
+        const char* getSender() const;
-        ObjectPath getPath() const;
+        const char* getPath() const;
-        ConnectionName getDestination() const;
+        const char* getDestination() const;
-        void peekType(std::string& type, std::string& contents) const;
+        // TODO: short docs in whole Message API
+        std::pair<char, const char*> peekType() const;
         bool isValid() const;
         bool isEmpty() const;
         bool isAtEnd(bool complete) const;
@@ -302,7 +300,8 @@ namespace sdbus {
 
     public:
         Signal() = default;
-        void setDestination(const ConnectionName& destination);
+        void setDestination(const std::string& destination);
+        void setDestination(const char* destination);
         void send() const;
     };
 
@@ -474,15 +473,14 @@ namespace sdbus {
     namespace detail
     {
         template <typename _Element, typename... _Elements>
-        bool deserialize_variant(Message& msg, std::variant<_Elements...>& value, const std::string& signature)
+        bool deserialize_variant(Message& msg, std::variant<_Elements...>& value, const char* signature)
         {
-            constexpr auto elemSignature = sdbus::signature_of_v<_Element>;
+            constexpr auto elemSignature = as_null_terminated(sdbus::signature_of_v<_Element>);
-            // TODO: Try to optimize
+            if (std::strcmp(signature, elemSignature.data()) != 0)
-            if (signature != std::string(elemSignature.begin(), elemSignature.end()))
                 return false;
 
             _Element temp;
-            msg.enterVariant(signature.c_str());
+            msg.enterVariant(signature);
             msg >> temp;
             msg.exitVariant();
             value = std::move(temp);
@@ -493,11 +491,8 @@ namespace sdbus {
     template <typename... Elements>
     inline Message& Message::operator>>(std::variant<Elements...>& value)
     {
-        std::string type;
+        auto [type, contents] = peekType();
-        std::string contentType;
+        bool result = (detail::deserialize_variant<Elements>(*this, value, contents) || ...);
-        // TODO: Refactor ppekType prior to release/v2.0 to return pair of const char* or string_view...
-        peekType(type, contentType);
-        bool result = (detail::deserialize_variant<Elements>(*this, value, contentType) || ...);
         SDBUS_THROW_ERROR_IF(!result, "Failed to deserialize variant: signature did not match any of the variant types", EINVAL);
         return *this;
     }

include/sdbus-c++/Types.h

@@ -30,6 +30,7 @@
 #include <sdbus-c++/Message.h>
 #include <sdbus-c++/TypeTraits.h>
 
+#include <cstring>
 #include <memory>
 #include <string>
 #include <tuple>
@@ -105,14 +106,14 @@ namespace sdbus {
         bool containsValueOfType() const
         {
             constexpr auto signature = as_null_terminated(signature_of_v<_Type>);
-            return signature.data() == peekValueType();
+            return std::strcmp(signature.data(), peekValueType()) == 0;
         }
 
         bool isEmpty() const;
 
         void serializeTo(Message& msg) const;
         void deserializeFrom(Message& msg);
-        std::string peekValueType() const;
+        const char* peekValueType() const;
 
     private:
         mutable PlainMessage msg_{};

src/Message.cpp

@@ -612,44 +612,38 @@ void Message::rewind(bool complete)
     SDBUS_THROW_ERROR_IF(r < 0, "Failed to rewind the message", -r);
 }
 
-InterfaceName Message::getInterfaceName() const
+const char* Message::getInterfaceName() const
 {
-    const auto* interface = sd_bus_message_get_interface((sd_bus_message*)msg_);
+    return sd_bus_message_get_interface((sd_bus_message*)msg_);
-    return interface != nullptr ? InterfaceName{interface} : InterfaceName{};
 }
 
-MemberName Message::getMemberName() const
+const char* Message::getMemberName() const
 {
-    const auto* member = sd_bus_message_get_member((sd_bus_message*)msg_);
+    return sd_bus_message_get_member((sd_bus_message*)msg_);
-    return member != nullptr ? MemberName{member} : MemberName{};
 }
 
-ConnectionName Message::getSender() const
+const char* Message::getSender() const
 {
-    const auto* sender = sd_bus_message_get_sender((sd_bus_message*)msg_);
+    return sd_bus_message_get_sender((sd_bus_message*)msg_);
-    return ConnectionName{sender};
 }
 
-ObjectPath Message::getPath() const
+const char* Message::getPath() const
 {
-    const auto* path = sd_bus_message_get_path((sd_bus_message*)msg_);
+    return sd_bus_message_get_path((sd_bus_message*)msg_);
-    return path != nullptr ? ObjectPath{path} : ObjectPath{};
 }
 
-ConnectionName Message::getDestination() const
+const char* Message::getDestination() const
 {
-    const auto* destination = sd_bus_message_get_destination((sd_bus_message*)msg_);
+    return sd_bus_message_get_destination((sd_bus_message*)msg_);
-    return destination != nullptr ? ConnectionName{destination} : ConnectionName{};
 }
 
-void Message::peekType(std::string& type, std::string& contents) const
+std::pair<char, const char*> Message::peekType() const
 {
-    char typeSig;
+    char typeSignature{};
-    const char* contentsSig;
+    const char* contentsSignature{};
-    auto r = sd_bus_message_peek_type((sd_bus_message*)msg_, &typeSig, &contentsSig);
+    auto r = sd_bus_message_peek_type((sd_bus_message*)msg_, &typeSignature, &contentsSignature);
     SDBUS_THROW_ERROR_IF(r < 0, "Failed to peek message type", -r);
-    type = typeSig;
+    return {typeSignature, contentsSignature};
-    contents = contentsSig ? contentsSig : "";
 }
 
 bool Message::isValid() const
@@ -877,9 +871,14 @@ void Signal::send() const
     SDBUS_THROW_ERROR_IF(r < 0, "Failed to emit signal", -r);
 }
 
-void Signal::setDestination(const ConnectionName& destination)
+void Signal::setDestination(const std::string& destination)
 {
-    auto r = sdbus_->sd_bus_message_set_destination((sd_bus_message*)msg_, destination.c_str());
+    return setDestination(destination.c_str());
+}
+
+void Signal::setDestination(const char* destination)
+{
+    auto r = sdbus_->sd_bus_message_set_destination((sd_bus_message*)msg_, destination);
     SDBUS_THROW_ERROR_IF(r < 0, "Failed to set signal destination", -r);
 }
 

src/Object.cpp

@@ -300,8 +300,6 @@ const Object::VTable::MethodItem* Object::findMethod(const VTable& vtable, std::
         return methodItem.name < methodName;
     });
 
-    (void)it;
-
     return it != vtable.methods.end() && it->name == methodName ? &*it : nullptr;
 }
 
@@ -312,8 +310,6 @@ const Object::VTable::PropertyItem* Object::findProperty(const VTable& vtable, s
         return propertyItem.name < propertyName;
     });
 
-    (void)it;
-
     return it != vtable.properties.end() && it->name == propertyName ? &*it : nullptr;
 }
 
@@ -333,7 +329,7 @@ int Object::sdbus_method_callback(sd_bus_message *sdbusMessage, void *userData,
 
     auto message = Message::Factory::create<MethodCall>(sdbusMessage, &vtable->object->connection_.getSdBusInterface());
 
-    const auto* methodItem = findMethod(*vtable, message.getMemberName()); // TODO: optimize the situation around getMemberName()
+    const auto* methodItem = findMethod(*vtable, message.getMemberName());
     assert(methodItem != nullptr);
     assert(methodItem->callback);
 

src/Proxy.cpp

@@ -113,7 +113,6 @@ PendingAsyncCall Proxy::callMethodAsync(const MethodCall& message, async_reply_h
 
     pendingAsyncCalls_.addCall(std::move(callData));
 
-    // TODO: Instead of PendingAsyncCall consider using Slot implementation for simplicity and consistency
     return {weakData};
 }
 

src/Types.cpp

@@ -55,12 +55,10 @@ void Variant::deserializeFrom(Message& msg)
     msg_.seal();
 }
 
-std::string Variant::peekValueType() const
+const char* Variant::peekValueType() const
 {
     msg_.rewind(false);
-    std::string type;
+    auto [type, contents] = msg_.peekType();
-    std::string contents;
-    msg_.peekType(type, contents);
     return contents;
 }
 

tests/integrationtests/DBusGeneralTests.cpp

@@ -44,6 +44,7 @@ using ::testing::ElementsAre;
 using ::testing::Eq;
 using namespace std::chrono_literals;
 using namespace sdbus::test;
+using namespace std::string_view_literals;
 
 using ADirectConnection = TestFixtureWithDirectConnection;
 
@@ -157,7 +158,7 @@ TYPED_TEST(AConnection, WillNotPassToMatchCallbackMessagesThatDoNotMatchTheRule)
     std::atomic<size_t> numberOfMatchingMessages{};
     auto slot = this->s_proxyConnection->addMatch(matchRule, [&](sdbus::Message msg)
     {
-        if(msg.getMemberName() == "simpleSignal")
+        if(msg.getMemberName() == "simpleSignal"sv)
             numberOfMatchingMessages++;
     });
     auto adaptor2 = std::make_unique<TestAdaptor>(*this->s_adaptorConnection, OBJECT_PATH_2);

tests/unittests/Message_test.cpp

@@ -32,8 +32,10 @@
 #include <list>
 
 using ::testing::Eq;
+using ::testing::StrEq;
 using ::testing::Gt;
 using ::testing::DoubleEq;
+using ::testing::IsNull;
 using namespace std::string_literals;
 
 namespace
@@ -465,11 +467,10 @@ TEST(AMessage, CanPeekASimpleType)
     msg << 123;
     msg.seal();
 
-    std::string type;
+    auto [type, contents] = msg.peekType();
-    std::string contents;
+
-    msg.peekType(type, contents);
+    ASSERT_THAT(type, Eq('i'));
-    ASSERT_THAT(type, "i");
+    ASSERT_THAT(contents, IsNull());
-    ASSERT_THAT(contents, "");
 }
 
 TEST(AMessage, CanPeekContainerContents)
@@ -478,11 +479,10 @@ TEST(AMessage, CanPeekContainerContents)
     msg << std::map<int, std::string>{{1, "one"}, {2, "two"}};
     msg.seal();
 
-    std::string type;
+    auto [type, contents] = msg.peekType();
-    std::string contents;
+
-    msg.peekType(type, contents);
+    ASSERT_THAT(type, Eq('a'));
-    ASSERT_THAT(type, "a");
+    ASSERT_THAT(contents, StrEq("{is}"));
-    ASSERT_THAT(contents, "{is}");
 }
 
 TEST(AMessage, CanCarryDBusArrayGivenAsCustomType)