diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1d038bccb..636fe81a0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -435,7 +435,7 @@ jobs: style: 'file' # read .clang-format for configuration tidy-checks: '' # Read .clang-tidy for configuration database: build/Release/compile_commands.json - version: 12 + version: 13 - name: Run linters on tests @@ -450,7 +450,7 @@ jobs: style: 'file' # read .clang-format for configuration tidy-checks: '' # Read .clang-tidy for configuration database: build/Release/compile_commands.json - version: 12 + version: 13 - name: Report lint failure diff --git a/include/up-cpp/client/usubscription/v3/Consumer.h b/include/up-cpp/client/usubscription/v3/Consumer.h index 8f4964d63..2fed6ab4b 100644 --- a/include/up-cpp/client/usubscription/v3/Consumer.h +++ b/include/up-cpp/client/usubscription/v3/Consumer.h @@ -9,8 +9,8 @@ // // SPDX-License-Identifier: Apache-2.0 -#ifndef UP_CPP_CLIENT_Consumer_H -#define UP_CPP_CLIENT_Consumer_H +#ifndef UP_CPP_CLIENT_USUBSCRIPTION_V3_CONSUMER_H +#define UP_CPP_CLIENT_USUBSCRIPTION_V3_CONSUMER_H #include #include @@ -23,8 +23,10 @@ #include namespace uprotocol::client::usubscription::v3 { -using namespace uprotocol::core::usubscription::v3; -using namespace uprotocol::utils; +using uprotocol::core::usubscription::v3::SubscriptionRequest; +using uprotocol::core::usubscription::v3::UnsubscribeRequest; +using uprotocol::core::usubscription::v3::Update; +using uprotocol::core::usubscription::v3::uSubscription; /** * @struct ConsumerOptions @@ -47,14 +49,13 @@ struct ConsumerOptions { std::optional subscription_details; }; - /// @struct uSubscriptionUUriBuilder /// @brief Structure to build uSubscription request URIs. /// /// This structure is used to build URIs for uSubscription service. It uses the -/// service options from uSubscription proto to set the authority name, ue_id, ue_version_major, and -/// the notification topic resource ID in the URI. -struct uSubscriptionUUriBuilder { +/// service options from uSubscription proto to set the authority name, ue_id, +/// ue_version_major, and the notification topic resource ID in the URI. +struct USubscriptionUUriBuilder { private: /// URI for the uSubscription service v1::UUri uri_; @@ -63,7 +64,7 @@ struct uSubscriptionUUriBuilder { public: /// @brief Constructor for uSubscriptionUUriBuilder. - uSubscriptionUUriBuilder() { + USubscriptionUUriBuilder() { // Get the service descriptor const google::protobuf::ServiceDescriptor* service = uSubscription::descriptor(); @@ -122,18 +123,20 @@ struct Consumer { /// /// @param transport Transport to register with. /// @param subscription_topic Topic to subscribe to. - /// @param callback Function that is called when publish message is received. + /// @param callback Function that is called when publish message is + /// received. /// @param priority Priority of the subscription request. /// @param subscribe_request_ttl Time to live for the subscription request. /// @param consumer_options Additional details for uSubscription service. [[nodiscard]] static ConsumerOrStatus create( std::shared_ptr transport, - const v1::UUri subscription_topic, ListenCallback&& callback, + const v1::UUri& subscription_topic, ListenCallback&& callback, v1::UPriority priority, std::chrono::milliseconds subscription_request_ttl, ConsumerOptions consumer_options); - /// @brief Unsubscribe from the topic and call uSubscription service to close the subscription. + /// @brief Unsubscribe from the topic and call uSubscription service to + /// close the subscription. /// /// @param priority Priority of the unsubscribe request. /// @param request_ttl Time to live for the unsubscribe request. @@ -156,7 +159,7 @@ struct Consumer { /// @param transport Transport to register with. /// @param subscriber_details Additional details about the subscriber. Consumer(std::shared_ptr transport, - const v1::UUri subscription_topic, + v1::UUri subscription_topic, ConsumerOptions consumer_options = {}); private: @@ -169,7 +172,7 @@ struct Consumer { ConsumerOptions consumer_options_; // URI info about the uSubscription service - uSubscriptionUUriBuilder uSubscriptionUUriBuilder_; + USubscriptionUUriBuilder uSubscriptionUUriBuilder_; // Subscription updates std::unique_ptr noficationSinkHandle_; @@ -205,8 +208,10 @@ struct Consumer { /// @brief Subscribe to the topic /// /// @param topic Topic to subscribe to. - /// @param subscription_request_ttl Time to live for the subscription request. - /// @param callback Function that is called when a published message is received. + /// @param subscription_request_ttl Time to live for the subscription + /// request. + /// @param callback Function that is called when a published message is + /// received. v1::UStatus subscribe(v1::UPriority priority, std::chrono::milliseconds subscription_request_ttl, ListenCallback&& callback); @@ -214,4 +219,4 @@ struct Consumer { } // namespace uprotocol::client::usubscription::v3 -#endif // UP_CPP_CLIENT_Consumer_H \ No newline at end of file +#endif // UP_CPP_CLIENT_USUBSCRIPTION_V3_CONSUMER_H \ No newline at end of file diff --git a/include/up-cpp/communication/NotificationSink.h b/include/up-cpp/communication/NotificationSink.h index 2e93b7ea5..007c4c3b8 100644 --- a/include/up-cpp/communication/NotificationSink.h +++ b/include/up-cpp/communication/NotificationSink.h @@ -58,7 +58,7 @@ struct NotificationSink { /// successfully. /// * UStatus containing an error state otherwise. [[nodiscard]] static SinkOrStatus create( - std::shared_ptr transport, + const std::shared_ptr& transport, ListenCallback&& callback, const v1::UUri& source_filter); /// @note DEPRECATED @@ -76,13 +76,12 @@ struct NotificationSink { [[deprecated( "See alternate overload of " "create()")]] [[nodiscard]] static SinkOrStatus - create(std::shared_ptr transport, + create(const std::shared_ptr& transport, const v1::UUri& sink, ListenCallback&& callback, std::optional&& source_filter); ~NotificationSink() = default; -protected: /// @brief Constructs a notification listener connected to a given /// transport. /// diff --git a/include/up-cpp/communication/RpcClient.h b/include/up-cpp/communication/RpcClient.h index 8ba7e8b0e..c78ab5f94 100644 --- a/include/up-cpp/communication/RpcClient.h +++ b/include/up-cpp/communication/RpcClient.h @@ -85,7 +85,7 @@ struct RpcClient { /// @name Passthroughs for std::future /// @{ auto get() { return future_.get(); } - auto valid() const noexcept { return future_.valid(); } + [[nodiscard]] auto valid() const noexcept { return future_.valid(); } void wait() const { future_.wait(); } template auto wait_for(Args&&... args) const { @@ -163,7 +163,7 @@ struct RpcClient { [[nodiscard]] InvokeFuture invokeMethod(); /// @brief Default move constructor (defined in RpcClient.cpp) - RpcClient(RpcClient&&); + RpcClient(RpcClient&&) noexcept; /// @brief Default destructor (defined in RpcClient.cpp) ~RpcClient(); diff --git a/include/up-cpp/communication/RpcServer.h b/include/up-cpp/communication/RpcServer.h index a5ff77103..daeebebec 100644 --- a/include/up-cpp/communication/RpcServer.h +++ b/include/up-cpp/communication/RpcServer.h @@ -90,9 +90,9 @@ struct RpcServer { /// @param ttl (Optional) Time response will be valid from the moment /// respond() is called. Note that the original request's TTL /// may also still apply. - RpcServer(std::shared_ptr transport, - std::optional format = {}, - std::optional ttl = {}); + explicit RpcServer(std::shared_ptr transport, + std::optional format = {}, + std::optional ttl = {}); /// @brief Allows std::make_unique to directly access RpcServer's private /// constructor. diff --git a/include/up-cpp/datamodel/builder/Payload.h b/include/up-cpp/datamodel/builder/Payload.h index 1fdd49fed..303b373a5 100644 --- a/include/up-cpp/datamodel/builder/Payload.h +++ b/include/up-cpp/datamodel/builder/Payload.h @@ -80,13 +80,13 @@ struct Payload { /// will compile out. /// @param data Data to be serialized and stored. template - Payload(Serializer s, const ValueT& data) { - auto serializedData = Serializer::serialize(data); + Payload(Serializer s [[maybe_unused]], const ValueT& data) { + auto serialized_data = Serializer::serialize(data); if (!UPayloadFormat_IsValid( - std::get(serializedData))) { + std::get(serialized_data))) { throw std::out_of_range("Invalid Serializer payload format"); } - payload_ = std::move(serializedData); + payload_ = std::move(serialized_data); } /// @brief Creates a Payload builder with a provided pre-serialized data. @@ -95,8 +95,7 @@ struct Payload { /// @param format The data format of the payload in value_bytes. /// /// @throws std::out_of_range If format is not valid for v1::UPayloadFormat - Payload(const std::vector& value_bytes, - const v1::UPayloadFormat format); + Payload(const std::vector& value_bytes, v1::UPayloadFormat format); /// @brief Creates a Payload builder with a provided pre-serialized data. /// @@ -107,7 +106,7 @@ struct Payload { /// /// @note This would typically be used for UPAYLOAD_FORMAT_TEXT or /// UPAYLOAD_FORMAT_JSON, but can be used for other payload formats. - Payload(const std::string& value, const v1::UPayloadFormat format); + Payload(const std::string& value, v1::UPayloadFormat format); /// @brief Creates a Payload builder with a provided pre-serialized data. /// @@ -120,7 +119,7 @@ struct Payload { /// /// @note This would typically be used for UPAYLOAD_FORMAT_TEXT or /// UPAYLOAD_FORMAT_JSON, but can be used for other payload formats. - Payload(std::string&& value, const v1::UPayloadFormat format); + Payload(std::string&& value, v1::UPayloadFormat format); /// @brief Creates a Payload builder with a provided pre-serialized data. /// diff --git a/include/up-cpp/datamodel/builder/UMessage.h b/include/up-cpp/datamodel/builder/UMessage.h index e9eaf2592..ae9e40d82 100644 --- a/include/up-cpp/datamodel/builder/UMessage.h +++ b/include/up-cpp/datamodel/builder/UMessage.h @@ -244,11 +244,11 @@ struct UMessageBuilder { private: /// @brief Constructs a UMessageBuilder with the provided attributes. /// - /// @param msgType + /// @param msg_type /// @param source /// @param sink /// @param request_id - UMessageBuilder(v1::UMessageType msgType, v1::UUri&& source, + UMessageBuilder(v1::UMessageType msg_type, v1::UUri&& source, std::optional&& sink = {}, std::optional&& request_id = {}); diff --git a/include/up-cpp/datamodel/builder/Uuid.h b/include/up-cpp/datamodel/builder/Uuid.h index 5a9421b25..da674a463 100644 --- a/include/up-cpp/datamodel/builder/Uuid.h +++ b/include/up-cpp/datamodel/builder/Uuid.h @@ -85,7 +85,7 @@ struct UuidBuilder { v1::UUID build(); private: - UuidBuilder(bool testing); + explicit UuidBuilder(bool testing); const bool testing_{false}; std::function time_source_; diff --git a/include/up-cpp/datamodel/constants/UuidConstants.h b/include/up-cpp/datamodel/constants/UuidConstants.h index fcd3e98b6..05eb7d663 100644 --- a/include/up-cpp/datamodel/constants/UuidConstants.h +++ b/include/up-cpp/datamodel/constants/UuidConstants.h @@ -38,6 +38,34 @@ constexpr uint64_t MASK_32_BITS = 0xFFFFFFFF; constexpr uint64_t MASK_16_BITS = 0xFFFF; constexpr uint64_t MASK_14_BITS = 0x3FFF; +// number of digits needed to represent a given number of bits in base 16 +constexpr uint64_t LEN_16_BITS_IN_HEX = 4; +constexpr uint64_t LEN_32_BITS_IN_HEX = 8; +constexpr uint64_t LEN_48_BITS_IN_HEX = 12; + +// number of characters a valid uuid +constexpr uint64_t TOTAL_UUID_LENGTH = 36; +constexpr uint64_t LEN_MSB_IN_HEX = 8; +constexpr uint64_t LEN_LSB_IN_HEX = 4; +constexpr uint64_t LEN_VCANT_IN_HEX = 4; +constexpr uint64_t LEN_VARR_IN_HEX = 4; +constexpr uint64_t LEN_RAND_IN_HEX = 8; + +// number of bits represented by a single hex character +constexpr uint64_t LEN_HEX_TO_BIT = 4; + +// number of bits to represent uint64 +constexpr uint64_t LEN_UINT64_IN_BIT = sizeof(uint64_t) * 8; + +// expected positions of the '-' separators in a valid uuid +constexpr uint64_t POS_FIRST_SEPARATOR = LEN_MSB_IN_HEX; +constexpr uint64_t POS_SECOND_SEPARATOR = + POS_FIRST_SEPARATOR + LEN_LSB_IN_HEX + 1; +constexpr uint64_t POS_THIRD_SEPARATOR = + POS_SECOND_SEPARATOR + LEN_VCANT_IN_HEX + 1; +constexpr uint64_t POS_FOURTH_SEPARATOR = + POS_THIRD_SEPARATOR + LEN_VARR_IN_HEX + 1; + } // namespace uprotocol::datamodel #endif // UP_CPP_DATAMODEL_CONSTANTS_UUIDCONSTANTS_H diff --git a/include/up-cpp/datamodel/serializer/Uuid.h b/include/up-cpp/datamodel/serializer/Uuid.h index b5f91dd1f..9641aafa1 100644 --- a/include/up-cpp/datamodel/serializer/Uuid.h +++ b/include/up-cpp/datamodel/serializer/Uuid.h @@ -27,13 +27,13 @@ namespace uprotocol::datamodel::serializer::uuid { /// @brief Converts to and from a human-readable string representation of UUID struct AsString { - [[nodiscard]] static std::string serialize(v1::UUID); + [[nodiscard]] static std::string serialize(const v1::UUID&); [[nodiscard]] static v1::UUID deserialize(const std::string&); }; /// @brief Converts to and from byte vector representation of UUID struct AsBytes { - [[nodiscard]] static std::vector serialize(v1::UUID); + [[nodiscard]] static std::vector serialize(const v1::UUID&); [[nodiscard]] static v1::UUID deserialize(const std::vector&); }; diff --git a/include/up-cpp/datamodel/validator/UUri.h b/include/up-cpp/datamodel/validator/UUri.h index 840b543fd..8989a5cf3 100644 --- a/include/up-cpp/datamodel/validator/UUri.h +++ b/include/up-cpp/datamodel/validator/UUri.h @@ -163,7 +163,7 @@ isValidDefaultSource(const v1::UUri&); struct InvalidUUri : public std::invalid_argument { // Forward constructors template - InvalidUUri(Args&&... args) + explicit InvalidUUri(Args&&... args) : std::invalid_argument(std::forward(args)...) {} InvalidUUri(InvalidUUri&&) noexcept; diff --git a/include/up-cpp/datamodel/validator/Uuid.h b/include/up-cpp/datamodel/validator/Uuid.h index ab5b0430a..b415af760 100644 --- a/include/up-cpp/datamodel/validator/Uuid.h +++ b/include/up-cpp/datamodel/validator/Uuid.h @@ -54,13 +54,13 @@ using ValidationResult = std::tuple>; /// @{ /// @brief Checks if the provided UUID contains valid uP v8 UUID data. /// @returns True if the UUID has valid UUID data, false otherwise. -ValidationResult isUuid(v1::UUID); +ValidationResult isUuid(const v1::UUID&); /// @brief Checks if the provided UUID has expired based on the given TTL. /// @throws InvalidUuid if the UUID does not contain valid UUID data /// @returns True if the difference between the current system time and /// the the timestamp in the UUID is greater than the TTL. -ValidationResult isExpired(v1::UUID uuid, std::chrono::milliseconds ttl); +ValidationResult isExpired(const v1::UUID& uuid, std::chrono::milliseconds ttl); /// @} /// @name Inspection utilities @@ -68,30 +68,30 @@ ValidationResult isExpired(v1::UUID uuid, std::chrono::milliseconds ttl); /// @brief Gets the version field from a UUID object /// @throws InvalidUuid if the UUID does not contain valid UUID data /// @returns The UUID's version -uint8_t getVersion(v1::UUID); +uint8_t getVersion(const v1::UUID&); /// @brief Gets the variant field from a UUID object /// @throws InvalidUuid if the UUID does not contain valid UUID data /// @returns The UUID's variant -uint8_t getVariant(v1::UUID); +uint8_t getVariant(const v1::UUID&); /// @brief Gets the timestamp field from a UUID object /// @throws InvalidUuid if the UUID does not contain valid UUID data /// @returns The UUID's timestamp as a chrono::time_point for the system /// clock. -std::chrono::system_clock::time_point getTime(v1::UUID uuid); +std::chrono::system_clock::time_point getTime(const v1::UUID& uuid); /// @brief Gets the difference between a UUID's timestamp and the current /// time according to the system clock/ /// @throws InvalidUuid if the UUID does not contain valid UUID data /// @returns The age of the UUID in milliseconds -std::chrono::milliseconds getElapsedTime(v1::UUID); +std::chrono::milliseconds getElapsedTime(const v1::UUID& uuid); /// @brief Gets the time remaining before the UUID expires, based on the /// given TTL. /// @throws InvalidUuid if the UUID does not contain valid UUID data /// @returns Remaining time (ttl - getElapsedTime(uuid)) in milliseconds -std::chrono::milliseconds getRemainingTime(v1::UUID uuid, +std::chrono::milliseconds getRemainingTime(const v1::UUID& uuid, std::chrono::milliseconds ttl); /// @} diff --git a/include/up-cpp/transport/UTransport.h b/include/up-cpp/transport/UTransport.h index 299f92c9e..cb54f96ea 100644 --- a/include/up-cpp/transport/UTransport.h +++ b/include/up-cpp/transport/UTransport.h @@ -57,7 +57,7 @@ class UTransport { /// /// @see uprotocol::datamodel::validator::uri::isValidEntityUri() /// @see uprotocol::datamodel::validator::uri::InvalidUUri - explicit UTransport(const v1::UUri&); + explicit UTransport(v1::UUri); /// @brief Send a message. /// @@ -264,7 +264,7 @@ class UTransport { /// connection they represent. /// /// @param listener CallerHandle for the connection that has been broken. - virtual void cleanupListener(CallableConn listener); + virtual void cleanupListener(const CallableConn& listener); private: /// @brief URI for the entity owning this transport. @@ -274,14 +274,15 @@ class UTransport { const v1::UUri entity_uri_; }; -struct NullTransport : public std::invalid_argument { +struct NullTransport : std::invalid_argument { template - NullTransport(Args&&... args) + explicit NullTransport(Args&&... args) : std::invalid_argument(std::forward(args)...) {} template - NullTransport operator=(Args&&... args) { - return std::invalid_argument::operator=(std::forward(args)...); + NullTransport& operator=(Args&&... args) { + std::invalid_argument::operator=(std::forward(args)...); + return *this; } }; diff --git a/include/up-cpp/utils/CallbackConnection.h b/include/up-cpp/utils/CallbackConnection.h index 417f342aa..b0dbda58d 100644 --- a/include/up-cpp/utils/CallbackConnection.h +++ b/include/up-cpp/utils/CallbackConnection.h @@ -173,7 +173,7 @@ struct [[nodiscard]] Connection { } if constexpr (!std::is_void_v) { - return result; + return static_cast>(std::move(result)); } } @@ -193,7 +193,8 @@ struct [[nodiscard]] Connection { }; /// @brief Semi-private constructor. Use the static establish() instead. - Connection(std::shared_ptr cb, PrivateConstructToken) + Connection(std::shared_ptr cb, + PrivateConstructToken token [[maybe_unused]]) : callback_(cb) {} // Connection is only ever available wrapped in a std::shared_ptr. @@ -230,7 +231,7 @@ struct [[nodiscard]] Connection { /// reason. struct BadConnection : public std::runtime_error { template - BadConnection(Args&&... args) + explicit BadConnection(Args&&... args) : std::runtime_error(std::forward(args)...) {} }; @@ -242,7 +243,7 @@ struct BadConnection : public std::runtime_error { /// the error appear earlier without waiting for invocation to occur. struct EmptyFunctionObject : public std::invalid_argument { template - EmptyFunctionObject(Args&&... args) + explicit EmptyFunctionObject(Args&&... args) : std::invalid_argument(std::forward(args)...) {} }; @@ -257,7 +258,7 @@ struct [[nodiscard]] CalleeHandle { CalleeHandle(std::shared_ptr connection, std::shared_ptr callback, std::optional&& cleanup, - typename Conn::PrivateConstructToken) + typename Conn::PrivateConstructToken token [[maybe_unused]]) : connection_(connection), callback_(callback), cleanup_(std::move(cleanup)) { @@ -333,7 +334,7 @@ struct [[nodiscard]] CalleeHandle { /// * False if the connection has been broken (i.e. This handle has /// been reset/moved, or all other references to the connection /// have been discarded) - bool isConnected() const { + [[nodiscard]] bool isConnected() const { auto locked_connection = connection_.lock(); return locked_connection && (*locked_connection); } @@ -351,7 +352,7 @@ struct [[nodiscard]] CalleeHandle { /// CallerHandle that needs to be corrected. struct BadCallerAccess : public std::logic_error { template - BadCallerAccess(Args&&... args) + explicit BadCallerAccess(Args&&... args) : std::logic_error(std::forward(args)...) {} }; @@ -364,7 +365,7 @@ struct [[nodiscard]] CallerHandle { /// @brief Creates a connected handle. Only usable by Connection CallerHandle(std::shared_ptr connection, - typename Conn::PrivateConstructToken) + typename Conn::PrivateConstructToken token [[maybe_unused]]) : connection_(connection) { if (!connection_) { throw BadConnection( @@ -390,7 +391,9 @@ struct [[nodiscard]] CallerHandle { /// * False if the connection has been broken (i.e. This handle has /// been reset/moved, or all other references to the connection /// have been discarded) - bool isConnected() const { return connection_ && (*connection_); } + [[nodiscard]] bool isConnected() const { + return connection_ && (*connection_); + } /// @throws BadCallerAccess if this handle has been default constructed OR /// reset() has left it without a valid conneciton pointer. @@ -440,7 +443,7 @@ struct InvokeResult { value_ = std::move(v); return *this; } - operator std::optional&&() && { return std::move(value_); } + explicit operator std::optional&&() && { return std::move(value_); } private: std::optional value_; diff --git a/include/up-cpp/utils/CyclicQueue.h b/include/up-cpp/utils/CyclicQueue.h index 2fc8ee13f..29739e94e 100644 --- a/include/up-cpp/utils/CyclicQueue.h +++ b/include/up-cpp/utils/CyclicQueue.h @@ -23,7 +23,7 @@ namespace uprotocol::utils { template class CyclicQueue final { public: - explicit CyclicQueue(const size_t max_size); + explicit CyclicQueue(size_t max_size); CyclicQueue(const CyclicQueue&) = delete; CyclicQueue& operator=(const CyclicQueue&) = delete; diff --git a/include/up-cpp/utils/Expected.h b/include/up-cpp/utils/Expected.h index 56e2e8ba8..a9a0482ae 100644 --- a/include/up-cpp/utils/Expected.h +++ b/include/up-cpp/utils/Expected.h @@ -31,7 +31,7 @@ static_assert(!__has_cpp_attribute(__cpp_lib_expected), /// @{ struct BadExpectedAccess : public std::runtime_error { template - BadExpectedAccess(Args&&... args) + explicit BadExpectedAccess(Args&&... args) : std::runtime_error(std::forward(args)...) {} }; @@ -41,7 +41,7 @@ template class Unexpected { public: constexpr Unexpected(const Unexpected&) = default; - constexpr Unexpected(Unexpected&&) = default; + constexpr Unexpected(Unexpected&&) noexcept = default; constexpr explicit Unexpected(const E& rhs) : storage_(rhs) {} constexpr explicit Unexpected(E&& rhs) : storage_(std::move(rhs)) {} @@ -57,14 +57,18 @@ class Unexpected { template class Expected { public: - template - constexpr Expected(Args&&... args) - : storage_(std::forward(args)...) {} + constexpr explicit Expected(T arg) : storage_(std::forward(arg)) {} + // It E and T are the same type, this can cause problems. Previously, this + // was in use by implicid conversion + // constexpr explicit Expected(E arg) : + // storage_(std::forward>(Unexpected(arg))) {} + constexpr explicit Expected(Unexpected arg) + : storage_(std::forward>(arg)) {} constexpr Expected(const Expected&) = default; - constexpr Expected(Expected&&) = default; + constexpr Expected(Expected&&) noexcept = default; - constexpr bool has_value() const noexcept { + [[nodiscard]] constexpr bool has_value() const noexcept { return std::holds_alternative(storage_); } @@ -79,44 +83,50 @@ class Expected { } constexpr const T& value() const& { - if (!has_value()) + if (!has_value()) { throw BadExpectedAccess( "Attempt to access value() when unexpected."); + } return std::get(storage_); } constexpr T value() && { - if (!has_value()) + if (!has_value()) { throw BadExpectedAccess( "Attempt to access value() when unexpected."); + } return std::move(std::get(storage_)); } constexpr const E& error() const& { - if (has_value()) + if (has_value()) { throw BadExpectedAccess( "Attempt to access error() when not unexpected."); + } return std::get>(storage_).error(); } constexpr E error() && { - if (has_value()) + if (has_value()) { throw BadExpectedAccess( "Attempt to access error() when not unexpected."); + } return std::move(std::get>(storage_)).error(); } constexpr const T& operator*() const { - if (!has_value()) + if (!has_value()) { throw BadExpectedAccess( "Attempt to dereference expected value when unexpected."); + } return std::get(storage_); } constexpr const T* operator->() const { - if (!has_value()) + if (!has_value()) { throw BadExpectedAccess( "Attempt to dereference expected pointer when unexpected."); + } return &std::get(storage_); } diff --git a/include/up-cpp/utils/IpAddress.h b/include/up-cpp/utils/IpAddress.h index b8ed8c735..420810474 100644 --- a/include/up-cpp/utils/IpAddress.h +++ b/include/up-cpp/utils/IpAddress.h @@ -59,7 +59,7 @@ struct IpAddress { /// @brief Constructs an IP address from a string representation of /// an address. - explicit IpAddress(std::string_view const ip_string); + explicit IpAddress(std::string_view ip_string); /// @brief Constructs an IP address from a binary representation of /// an address. @@ -83,10 +83,10 @@ struct IpAddress { [[nodiscard]] std::string getBytesString() const; /// @brief Number of bytes in IPv4 address. - static constexpr uint8_t ip_v4_address_bytes = 4; + static constexpr uint8_t IP_V4_ADDRESS_BYTES = 4; /// @brief Number of bytes in IPv6 address. - static constexpr uint8_t ip_v6_address_bytes = 16; + static constexpr uint8_t IP_V6_ADDRESS_BYTES = 16; private: /// @brief Updates the state of this instance from the value of the diff --git a/include/up-cpp/utils/ProtoConverter.h b/include/up-cpp/utils/ProtoConverter.h index 987dd3c4d..ee4123936 100644 --- a/include/up-cpp/utils/ProtoConverter.h +++ b/include/up-cpp/utils/ProtoConverter.h @@ -1,5 +1,5 @@ -#ifndef PROTO_CONVERTER_H -#define PROTO_CONVERTER_H +#ifndef UP_CPP_UTILS_PROTOCONVERTER_H +#define UP_CPP_UTILS_PROTOCONVERTER_H #include #include @@ -8,7 +8,10 @@ #include namespace uprotocol::utils { -using namespace uprotocol::core::usubscription::v3; +using uprotocol::core::usubscription::v3::SubscribeAttributes; +using uprotocol::core::usubscription::v3::SubscriberInfo; +using uprotocol::core::usubscription::v3::SubscriptionRequest; +using uprotocol::core::usubscription::v3::UnsubscribeRequest; struct ProtoConverter { /// @brief Converts std::chrono::time_point to google::protobuf::Timestamp @@ -51,5 +54,5 @@ struct ProtoConverter { static UnsubscribeRequest BuildUnSubscribeRequest( const v1::UUri& subscription_topic); }; -}; // namespace uprotocol::utils -#endif // PROTO_CONVERTER_H +}; // namespace uprotocol::utils +#endif // UP_CPP_UTILS_PROTOCONVERTER_H diff --git a/include/up-cpp/utils/ThreadPool.h b/include/up-cpp/utils/ThreadPool.h index d1d6cde46..f888509cf 100644 --- a/include/up-cpp/utils/ThreadPool.h +++ b/include/up-cpp/utils/ThreadPool.h @@ -32,7 +32,7 @@ class ThreadPool { ThreadPool& operator=(const ThreadPool&) = delete; ThreadPool& operator=(ThreadPool&&) = delete; - ThreadPool(const size_t max_queue_size, const size_t max_num_of_threads, + ThreadPool(size_t max_queue_size, size_t max_num_of_threads, std::chrono::milliseconds task_timeout); ~ThreadPool(); diff --git a/include/up-cpp/utils/base64.h b/include/up-cpp/utils/base64.h index 88453460e..77b2cf7bf 100644 --- a/include/up-cpp/utils/base64.h +++ b/include/up-cpp/utils/base64.h @@ -18,7 +18,7 @@ #include /// @brief Utilities for encoding / decoding data in Base 64 format. -namespace uprotocol::utils::Base64 { +namespace uprotocol::utils::base64 { /// @brief Encode a string of bytes to base64. std::string encode(std::string_view); @@ -44,6 +44,6 @@ size_t encodedLen(std::vector); /// decoded data. size_t decodedLen(std::string_view); -} // namespace uprotocol::utils::Base64 +} // namespace uprotocol::utils::base64 #endif // UP_CPP_UTILS_BASE64_H diff --git a/lint/clang-format.sh b/lint/clang-format.sh index afaf1c838..de7e64181 100755 --- a/lint/clang-format.sh +++ b/lint/clang-format.sh @@ -2,11 +2,11 @@ PROJECT_ROOT="$(realpath "$(dirname "$0")/../")" -if [ -n "$(which clang-format-12)" ]; then - # NOTE: Using clang-format-12 in CI system, too - FORMATTER=clang-format-12 +if [ -n "$(which clang-format-13)" ]; then + # NOTE: Using clang-format-13 in CI system, too + FORMATTER=clang-format-13 elif [ -n "$(which clang-format)" ]; then - echo "Did not find clang-format-12. Trying clang-format. Results may not" + echo "Did not find clang-format-13. Trying clang-format. Results may not" echo "match formatting in GitHub CI process." FORMATTER=clang-format else diff --git a/lint/clang-tidy.sh b/lint/clang-tidy.sh index 67e9242e8..e2b490758 100755 --- a/lint/clang-tidy.sh +++ b/lint/clang-tidy.sh @@ -2,11 +2,11 @@ PROJECT_ROOT="$(realpath "$(dirname "$0")/../")" -if [ -n "$(which clang-tidy-12)" ]; then - # NOTE: Using clang-tidy-12 in CI system, too - LINTER=clang-tidy-12 +if [ -n "$(which clang-tidy-13)" ]; then + # NOTE: Using clang-tidy-13 in CI system, too + LINTER=clang-tidy-13 elif [ -n "$(which clang-tidy)" ]; then - echo "Did not find clang-tidy-12. Trying clang-tidy. Results may not" + echo "Did not find clang-tidy-13. Trying clang-tidy. Results may not" echo "match formatting in GitHub CI process." LINTER=clang-tidy else @@ -58,7 +58,7 @@ if [ -z "$target_source" ]; then shopt -s globstar pushd "$PROJECT_ROOT" > /dev/null - for f in **/*.h **/*.cpp; do + for f in include/**/*.h src/**/*.cpp; do # test/coverage/**/*.cpp test/extra/**/*.cpp test/include/**/*.h; do if [[ ! ("$f" =~ "build/") ]]; then echo echo "Checking file '$f'" diff --git a/src/client/usubscription/v3/Consumer.cpp b/src/client/usubscription/v3/Consumer.cpp index a968627a0..0217b63ea 100644 --- a/src/client/usubscription/v3/Consumer.cpp +++ b/src/client/usubscription/v3/Consumer.cpp @@ -11,22 +11,24 @@ #include +#include + namespace uprotocol::client::usubscription::v3 { Consumer::Consumer(std::shared_ptr transport, - const v1::UUri subscription_topic, + v1::UUri subscription_topic, ConsumerOptions consumer_options) - : transport_(transport), - subscription_topic_(subscription_topic), - consumer_options_(consumer_options) { + : transport_(std::move(transport)), + subscription_topic_(std::move(subscription_topic)), + consumer_options_(std::move(consumer_options)), + rpc_client_(nullptr) { // Initialize uSubscriptionUUriBuilder_ - uSubscriptionUUriBuilder_ = uSubscriptionUUriBuilder(); - rpc_client_ = nullptr; + uSubscriptionUUriBuilder_ = USubscriptionUUriBuilder(); } [[nodiscard]] Consumer::ConsumerOrStatus Consumer::create( std::shared_ptr transport, - const v1::UUri subscription_topic, ListenCallback&& callback, + const v1::UUri& subscription_topic, ListenCallback&& callback, v1::UPriority priority, std::chrono::milliseconds subscription_request_ttl, ConsumerOptions consumer_options) { auto consumer = std::make_unique( @@ -40,14 +42,12 @@ Consumer::Consumer(std::shared_ptr transport, status = consumer->subscribe(priority, subscription_request_ttl, std::move(callback)); if (status.code() == v1::UCode::OK) { - return consumer; - } else { - return uprotocol::utils::Unexpected(std::move(status)); + return ConsumerOrStatus(std::move(consumer)); } - } else { - // If connection fails, return the error status. - return uprotocol::utils::Unexpected(std::move(status)); + return ConsumerOrStatus(utils::Unexpected(status)); } + // If connection fails, return the error status. + return ConsumerOrStatus(utils::Unexpected(status)); } v1::UStatus Consumer::createNotificationSink() { @@ -66,25 +66,23 @@ v1::UStatus Consumer::createNotificationSink() { auto notification_topic = uSubscriptionUUriBuilder_.getNotificationUri(); auto result = communication::NotificationSink::create( - transport_, std::move(notification_sink_callback), - std::move(notification_topic)); + transport_, std::move(notification_sink_callback), notification_topic); if (result.has_value()) { noficationSinkHandle_ = std::move(result).value(); v1::UStatus status; status.set_code(v1::UCode::OK); return status; - } else { - return result.error(); } + return result.error(); } SubscriptionRequest Consumer::buildSubscriptionRequest() { - auto attributes = ProtoConverter::BuildSubscribeAttributes( + auto attributes = utils::ProtoConverter::BuildSubscribeAttributes( consumer_options_.when_expire, consumer_options_.subscription_details, consumer_options_.sample_period_ms); - auto subscription_request = ProtoConverter::BuildSubscriptionRequest( + auto subscription_request = utils::ProtoConverter::BuildSubscriptionRequest( subscription_topic_, attributes); return subscription_request; } @@ -93,14 +91,14 @@ v1::UStatus Consumer::subscribe( v1::UPriority priority, std::chrono::milliseconds subscription_request_ttl, ListenCallback&& callback) { rpc_client_ = std::make_unique( - transport_, - uSubscriptionUUriBuilder_.getServiceUriWithResourceId(1), + transport_, uSubscriptionUUriBuilder_.getServiceUriWithResourceId(1), priority, subscription_request_ttl); - auto onResponse = [this](auto maybeResponse) { - if (maybeResponse.has_value() && maybeResponse.value().has_payload()) { + auto on_response = [this](const auto& maybe_response) { + if (maybe_response.has_value() && + maybe_response.value().has_payload()) { SubscriptionResponse response; - if (response.ParseFromString(maybeResponse.value().payload())) { + if (response.ParseFromString(maybe_response.value().payload())) { if (response.topic().SerializeAsString() == subscription_topic_.SerializeAsString()) { subscription_response_ = response; @@ -109,11 +107,11 @@ v1::UStatus Consumer::subscribe( } }; - SubscriptionRequest subscriptionRequest = buildSubscriptionRequest(); - auto payload = datamodel::builder::Payload(std::move(subscriptionRequest)); + SubscriptionRequest const subscription_request = buildSubscriptionRequest(); + auto payload = datamodel::builder::Payload(subscription_request); rpc_handle_ = - rpc_client_->invokeMethod(std::move(payload), std::move(onResponse)); + rpc_client_->invokeMethod(std::move(payload), std::move(on_response)); // Create a L2 subscription auto result = communication::Subscriber::subscribe( @@ -124,14 +122,13 @@ v1::UStatus Consumer::subscribe( v1::UStatus status; status.set_code(v1::UCode::OK); return status; - } else { - return result.error(); } + return result.error(); } UnsubscribeRequest Consumer::buildUnsubscriptionRequest() { auto unsubscribe_request = - ProtoConverter::BuildUnSubscribeRequest(subscription_topic_); + utils::ProtoConverter::BuildUnSubscribeRequest(subscription_topic_); return unsubscribe_request; } @@ -141,17 +138,17 @@ void Consumer::unsubscribe(v1::UPriority priority, transport_, uSubscriptionUUriBuilder_.getServiceUriWithResourceId(2), priority, request_ttl); - auto onResponse = [](auto maybeResponse) { - if (!maybeResponse.has_value()) { + auto on_response = [](const auto& maybe_response) { + if (!maybe_response.has_value()) { // Do something as this means sucessfully unsubscribed. } }; - UnsubscribeRequest unsubscribeRequest = buildUnsubscriptionRequest(); - auto payload = datamodel::builder::Payload(std::move(unsubscribeRequest)); + UnsubscribeRequest const unsubscribe_request = buildUnsubscriptionRequest(); + auto payload = datamodel::builder::Payload(unsubscribe_request); rpc_handle_ = - rpc_client_->invokeMethod(std::move(payload), std::move(onResponse)); + rpc_client_->invokeMethod(std::move(payload), std::move(on_response)); subscriber_.reset(); } diff --git a/src/communication/NotificationSink.cpp b/src/communication/NotificationSink.cpp index 27d7bafa4..08043f3d6 100644 --- a/src/communication/NotificationSink.cpp +++ b/src/communication/NotificationSink.cpp @@ -19,8 +19,8 @@ namespace uprotocol::communication { namespace UriValidator = datamodel::validator::uri; NotificationSink::SinkOrStatus NotificationSink::create( - std::shared_ptr transport, ListenCallback&& callback, - const uprotocol::v1::UUri& source_filter) { + const std::shared_ptr& transport, + ListenCallback&& callback, const uprotocol::v1::UUri& source_filter) { // Standard check - transport pointer cannot be null if (!transport) { throw transport::NullTransport("transport cannot be null"); @@ -40,17 +40,16 @@ NotificationSink::SinkOrStatus NotificationSink::create( std::move(callback), source_filter, transport->getEntityUri()); if (!listener) { - return uprotocol::utils::Unexpected(listener.error()); + return SinkOrStatus(utils::Unexpected(listener.error())); } - return std::make_unique( - std::forward>(transport), - std::forward(std::move(listener).value())); + return SinkOrStatus(std::make_unique( + transport, std::forward(std::move(listener).value()))); } // NOTE: deprecated NotificationSink::SinkOrStatus NotificationSink::create( - std::shared_ptr transport, + const std::shared_ptr& transport, const uprotocol::v1::UUri& sink, ListenCallback&& callback, std::optional&& source_filter) { // Standard check - transport pointer cannot be null diff --git a/src/communication/NotificationSource.cpp b/src/communication/NotificationSource.cpp index 262320312..c5cbb863b 100644 --- a/src/communication/NotificationSource.cpp +++ b/src/communication/NotificationSource.cpp @@ -12,7 +12,8 @@ #include "up-cpp/communication/NotificationSource.h" namespace uprotocol::communication { -using namespace uprotocol::datamodel::builder; + +using uprotocol::datamodel::builder::UMessageBuilder; NotificationSource::NotificationSource( std::shared_ptr transport, v1::UUri&& source, @@ -40,7 +41,7 @@ v1::UStatus NotificationSource::notify( datamodel::builder::Payload&& payload) const { auto message = notify_builder_.build(std::move(payload)); - return transport_->send(std::move(message)); + return transport_->send(message); } v1::UStatus NotificationSource::notify() const { @@ -49,7 +50,7 @@ v1::UStatus NotificationSource::notify() const { throw transport::NullTransport("transport cannot be null"); } - return transport_->send(std::move(message)); + return transport_->send(message); } } // namespace uprotocol::communication \ No newline at end of file diff --git a/src/communication/Publisher.cpp b/src/communication/Publisher.cpp index 5bf30f7e7..55a055caa 100644 --- a/src/communication/Publisher.cpp +++ b/src/communication/Publisher.cpp @@ -14,7 +14,7 @@ #include namespace uprotocol::communication { -using namespace uprotocol::datamodel::builder; +using uprotocol::datamodel::builder::UMessageBuilder; Publisher::Publisher(std::shared_ptr transport, v1::UUri&& topic, v1::UPayloadFormat format, @@ -40,7 +40,7 @@ v1::UStatus Publisher::publish(datamodel::builder::Payload&& payload) const { throw transport::NullTransport("transport cannot be null"); } - return transport_->send(std::move(message)); + return transport_->send(message); } } // namespace uprotocol::communication \ No newline at end of file diff --git a/src/communication/RpcClient.cpp b/src/communication/RpcClient.cpp index 4a0b061bf..87239a71f 100644 --- a/src/communication/RpcClient.cpp +++ b/src/communication/RpcClient.cpp @@ -15,24 +15,39 @@ #include #include +#include namespace { namespace detail { -using namespace uprotocol; +using uprotocol::v1::UStatus; +using ListenHandle = uprotocol::transport::UTransport::ListenHandle; struct PendingRequest { - std::chrono::steady_clock::time_point when_expire; - transport::UTransport::ListenHandle response_listener; - std::function expire; - size_t instance_id; + friend struct ScrubablePendingQueue; + friend struct ExpireWorker; + + PendingRequest(const std::chrono::steady_clock::time_point& when_expire, + ListenHandle response_listener, + std::function expire, + const size_t& instance_id) + : when_expire_(when_expire), + response_listener_(std::move(response_listener)), + expire_(std::move(expire)), + instance_id_(instance_id) {} auto operator>(const PendingRequest& other) const; + +private: + std::chrono::steady_clock::time_point when_expire_; + ListenHandle response_listener_; + std::function expire_; + size_t instance_id_{}; }; struct ScrubablePendingQueue : public std::priority_queue, - std::greater> { + std::greater<>> { ~ScrubablePendingQueue(); auto scrub(size_t instance_id); PendingRequest& top(); @@ -41,7 +56,7 @@ struct ScrubablePendingQueue struct ExpireWorker { ExpireWorker(); ~ExpireWorker(); - void enqueue(PendingRequest&& request); + void enqueue(PendingRequest&& pending); void scrub(size_t instance_id); void doWork(); @@ -60,25 +75,25 @@ namespace uprotocol::communication { //////////////////////////////////////////////////////////////////////////////// struct RpcClient::ExpireService { - ExpireService() : instance_id_(next_instance_id++) {} + ExpireService() : instance_id_(next_instance_id_++) {} - ~ExpireService() { worker.scrub(instance_id_); } + ~ExpireService() { worker_.scrub(instance_id_); } void enqueue(std::chrono::steady_clock::time_point when_expire, transport::UTransport::ListenHandle&& response_listener, - std::function expire) { - detail::PendingRequest pending; - pending.when_expire = when_expire; - pending.response_listener = std::move(response_listener); - pending.expire = std::move(expire); - pending.instance_id = instance_id_; - - worker.enqueue(std::move(pending)); + std::function expire) const { + auto pending = + detail::PendingRequest(when_expire, std::move(response_listener), + std::move(expire), instance_id_); + + worker_.enqueue(std::move(pending)); } private: - static inline std::atomic next_instance_id{0}; - static inline detail::ExpireWorker worker; + static inline std::atomic next_instance_id_{0}; + // constructor for ExpireWorker can throw an exception when trying to create + // new thread this can be problematic when used in a static constructor + static inline detail::ExpireWorker worker_; // NOLINT size_t instance_id_; }; @@ -89,7 +104,7 @@ RpcClient::RpcClient(std::shared_ptr transport, std::optional payload_format, std::optional permission_level, std::optional token) - : transport_(transport), + : transport_(std::move(transport)), ttl_(ttl), builder_(datamodel::builder::UMessageBuilder::request( std::move(method), v1::UUri(transport_->getEntityUri()), priority, @@ -124,8 +139,9 @@ RpcClient::InvokeHandle RpcClient::invokeMethod(v1::UMessage&& request, // attempt to call the callback succeeds. auto callback_once = std::make_shared(); - auto [callback_handle, callable] = - Connection::establish(std::move(callback)); + auto connected_pair = Connection::establish(std::move(callback)); + auto callback_handle = std::move(std::get<0>(connected_pair)); + auto callable = std::get<1>(connected_pair); /////////////////////////////////////////////////////////////////////////// // Wraps the callback to handle receive filtering and commstatus checking. @@ -145,10 +161,11 @@ RpcClient::InvokeHandle RpcClient::invokeMethod(v1::UMessage&& request, v1::UStatus status; status.set_code(m.attributes().commstatus()); status.set_message("Received response with !OK commstatus"); - std::call_once(*callback_once, [&callable, - status = std::move(status)]() { - callable(utils::Unexpected(std::move(status))); - }); + std::call_once( + *callback_once, [&callable, status = std::move(status)]() { + callable(utils::Expected( + utils::Unexpected(status))); + }); } } }; @@ -158,10 +175,11 @@ RpcClient::InvokeHandle RpcClient::invokeMethod(v1::UMessage&& request, // Called when the request has expired or failed. Will be handed off to the // expiration monitoring service once the request has been sent. auto expire = [callable, callback_once](v1::UStatus&& reason) mutable { - std::call_once( - *callback_once, [&callable, reason = std::move(reason)]() { - callable(utils::Unexpected(std::move(reason))); - }); + std::call_once(*callback_once, + [&callable, reason = std::move(reason)]() { + callable(utils::Expected( + utils::Unexpected(reason))); + }); }; /////////////////////////////////////////////////////////////////////////// @@ -182,7 +200,7 @@ RpcClient::InvokeHandle RpcClient::invokeMethod(v1::UMessage&& request, } } - return std::move(callback_handle); + return callback_handle; } RpcClient::InvokeHandle RpcClient::invokeMethod( @@ -203,10 +221,11 @@ RpcClient::InvokeFuture RpcClient::invokeMethod( // allows exactly one call to the callback via std::call_once. auto promise = std::make_shared>(); auto future = promise->get_future(); - auto handle = invokeMethod( - std::move(payload), [promise](MessageOrStatus maybe_message) mutable { - promise->set_value(maybe_message); - }); + auto handle = + invokeMethod(std::move(payload), + [promise](const MessageOrStatus& maybe_message) mutable { + promise->set_value(maybe_message); + }); return {std::move(future), std::move(handle)}; } @@ -219,14 +238,14 @@ RpcClient::InvokeFuture RpcClient::invokeMethod() { auto promise = std::make_shared>(); auto future = promise->get_future(); auto handle = - invokeMethod([promise](MessageOrStatus maybe_message) mutable { + invokeMethod([promise](const MessageOrStatus& maybe_message) mutable { promise->set_value(maybe_message); }); return {std::move(future), std::move(handle)}; } -RpcClient::RpcClient(RpcClient&&) = default; +RpcClient::RpcClient(RpcClient&&) noexcept = default; RpcClient::~RpcClient() = default; RpcClient::InvokeFuture::InvokeFuture() = default; @@ -244,17 +263,19 @@ RpcClient::InvokeFuture::InvokeFuture(std::future&& future, namespace { namespace detail { -using namespace uprotocol; -using namespace std::chrono_literals; +using uprotocol::v1::UCode; +using uprotocol::v1::UStatus; +// using namespace std::chrono_literals; +using ListenHandle = uprotocol::transport::UTransport::ListenHandle; auto PendingRequest::operator>(const PendingRequest& other) const { - return when_expire > other.when_expire; + return when_expire_ > other.when_expire_; } ScrubablePendingQueue::~ScrubablePendingQueue() { - const v1::UStatus cancel_reason = []() { - v1::UStatus reason; - reason.set_code(v1::UCode::INTERNAL); + const UStatus cancel_reason = []() { + UStatus reason; + reason.set_code(UCode::INTERNAL); reason.set_message( "ERROR: ExpireWorker has shut down while requests are still " "pending. This should never occur and likely indicates that an " @@ -263,30 +284,30 @@ ScrubablePendingQueue::~ScrubablePendingQueue() { }(); for (auto& pending : c) { - pending.expire(cancel_reason); + pending.expire_(cancel_reason); } } auto ScrubablePendingQueue::scrub(size_t instance_id) { // Collect all the expire lambdas so they can be called without the // lock held. - std::vector> all_expired; + std::vector> all_expired; c.erase( std::remove_if(c.begin(), c.end(), [instance_id, &all_expired](const PendingRequest& p) { - if (instance_id == p.instance_id) { - all_expired.push_back(p.expire); + if (instance_id == p.instance_id_) { + all_expired.push_back(p.expire_); return true; } return false; }), c.end()); - // TODO - is there a better way to shrink the internal container? - // Maybe instead we should enforce a capacity limit - constexpr size_t capacity_shrink_threshold = 16; - if ((c.capacity() > capacity_shrink_threshold) && + // TODO(missing_author) - is there a better way to shrink the internal + // container? Maybe instead we should enforce a capacity limit + constexpr size_t CAPACITY_SHRINK_THRESHOLD = 16; + if ((c.capacity() > CAPACITY_SHRINK_THRESHOLD) && (c.size() < c.capacity() / 2)) { c.shrink_to_fit(); } @@ -304,29 +325,29 @@ ExpireWorker::ExpireWorker() { ExpireWorker::~ExpireWorker() { stop_ = true; { - std::lock_guard lock(pending_mtx_); + std::lock_guard const lock(pending_mtx_); wake_worker_.notify_one(); } worker_.join(); } void ExpireWorker::enqueue(PendingRequest&& pending) { - std::lock_guard lock(pending_mtx_); + std::lock_guard const lock(pending_mtx_); pending_.emplace(std::move(pending)); wake_worker_.notify_one(); } void ExpireWorker::scrub(size_t instance_id) { - std::vector> all_expired; + std::vector> all_expired; { - std::lock_guard lock(pending_mtx_); + std::lock_guard const lock(pending_mtx_); all_expired = pending_.scrub(instance_id); wake_worker_.notify_one(); } - static const v1::UStatus cancel_reason = []() { - v1::UStatus reason; - reason.set_code(v1::UCode::CANCELLED); + static const UStatus cancel_reason = []() { + UStatus reason; + reason.set_code(UCode::CANCELLED); reason.set_message("RpcClient for this request was discarded"); return reason; }(); @@ -339,17 +360,17 @@ void ExpireWorker::scrub(size_t instance_id) { void ExpireWorker::doWork() { while (!stop_) { const auto now = std::chrono::steady_clock::now(); - std::optional maybe_expire; + std::optional maybe_expire; { - transport::UTransport::ListenHandle expired_handle; - std::lock_guard lock(pending_mtx_); + ListenHandle expired_handle; + std::lock_guard const lock(pending_mtx_); if (!pending_.empty()) { - const auto when_expire = pending_.top().when_expire; + const auto when_expire = pending_.top().when_expire_; if (when_expire <= now) { - maybe_expire = std::move(pending_.top().expire); + maybe_expire = std::move(pending_.top().expire_); expired_handle = - std::move(pending_.top().response_listener); + std::move(pending_.top().response_listener_); pending_.pop(); } } @@ -358,9 +379,9 @@ void ExpireWorker::doWork() { if (maybe_expire) { auto& expire = *maybe_expire; - static const v1::UStatus expire_reason = []() { - v1::UStatus reason; - reason.set_code(v1::UCode::DEADLINE_EXCEEDED); + static const UStatus expire_reason = []() { + UStatus reason; + reason.set_code(UCode::DEADLINE_EXCEEDED); reason.set_message("Request expired before response received"); return reason; }(); @@ -379,11 +400,11 @@ void ExpireWorker::doWork() { // priority queue (either by insertion or deletion) // * The queue has been emptied (loop back to indefinite sleep) // * A stop has been requested - auto wake_when = pending_.top().when_expire; + auto wake_when = pending_.top().when_expire_; wake_worker_.wait_until(lock, wake_when, [this, &wake_when]() { auto when_next_wake = wake_when; if (!pending_.empty()) { - when_next_wake = pending_.top().when_expire; + when_next_wake = pending_.top().when_expire_; } return stop_ || when_next_wake != wake_when || pending_.empty() || diff --git a/src/communication/RpcServer.cpp b/src/communication/RpcServer.cpp index a56b7489f..54c540417 100644 --- a/src/communication/RpcServer.cpp +++ b/src/communication/RpcServer.cpp @@ -20,7 +20,7 @@ RpcServer::RpcServer(std::shared_ptr transport, std::optional ttl) : transport_(std::move(transport)), ttl_(ttl), - expected_payload_format_(std::move(format)) { + expected_payload_format_(format) { if (!transport_) { throw transport::NullTransport("transport cannot be null"); } @@ -43,7 +43,7 @@ RpcServer::ServerOrStatus RpcServer::create( v1::UStatus status; status.set_code(v1::UCode::INVALID_ARGUMENT); status.set_message("Invalid rpc URI"); - return uprotocol::utils::Unexpected(status); + return ServerOrStatus(utils::Unexpected(status)); } // Validate the payload format, if provided. @@ -53,7 +53,7 @@ RpcServer::ServerOrStatus RpcServer::create( v1::UStatus status; status.set_code(v1::UCode::OUT_OF_RANGE); status.set_message("Invalid payload format"); - return uprotocol::utils::Unexpected(status); + return ServerOrStatus(utils::Unexpected(status)); } } @@ -67,11 +67,10 @@ RpcServer::ServerOrStatus RpcServer::create( auto status = server->connect(method_name, std::move(callback)); if (status.code() == v1::UCode::OK) { // If connection is successful, return the server instance. - return server; - } else { - // If connection fails, return the error status. - return uprotocol::utils::Unexpected(std::move(status)); + return ServerOrStatus(std::move(server)); } + // If connection fails, return the error status. + return ServerOrStatus(utils::Unexpected(status)); } v1::UStatus RpcServer::connect(const v1::UUri& method, RpcCallback&& callback) { @@ -96,7 +95,7 @@ v1::UStatus RpcServer::connect(const v1::UUri& method, RpcCallback&& callback) { datamodel::builder::UMessageBuilder::response(request); // Call the RPC callback method with the request message. - auto payloadData = callback_(request); + auto payload_data = callback_(request); if (ttl_.has_value()) { builder.withTtl(ttl_.value()); @@ -108,7 +107,7 @@ v1::UStatus RpcServer::connect(const v1::UUri& method, RpcCallback&& callback) { // Check for payload data requirement based on expected format // presence - if (!payloadData.has_value()) { + if (!payload_data.has_value()) { // builder.build() verifies if payload format is required auto response = builder.build(); // Ignoring status code for transport send @@ -116,7 +115,7 @@ v1::UStatus RpcServer::connect(const v1::UUri& method, RpcCallback&& callback) { } else { // builder.build(payloadData) verifies if payload format // matches the expected - auto response = builder.build(std::move(payloadData).value()); + auto response = builder.build(std::move(payload_data).value()); // Ignoring status code for transport send std::ignore = transport_->send(response); } @@ -126,9 +125,13 @@ v1::UStatus RpcServer::connect(const v1::UUri& method, RpcCallback&& callback) { v1::UUri any_uri; any_uri.set_authority_name("*"); // Instance ID 0 and UE ID FFFF for wildcard - any_uri.set_ue_id(0x0000FFFF); - any_uri.set_ue_version_major(0xFF); - any_uri.set_resource_id(0xFFFF); + constexpr auto DEFAULT_INSTANCE_ID_WITH_WILDCARD_SERVICE_ID = + 0x0000FFFF; + constexpr auto VERSION_MAJOR_WILDCARD = 0xFF; + constexpr auto RESOURCE_ID_WILDCARD = 0xFFFF; + any_uri.set_ue_id(DEFAULT_INSTANCE_ID_WITH_WILDCARD_SERVICE_ID); + any_uri.set_ue_version_major(VERSION_MAJOR_WILDCARD); + any_uri.set_resource_id(RESOURCE_ID_WILDCARD); return any_uri; }(), // sink_filter= @@ -141,9 +144,8 @@ v1::UStatus RpcServer::connect(const v1::UUri& method, RpcCallback&& callback) { v1::UStatus status; status.set_code(v1::UCode::OK); return status; - } else { - return result.error(); } + return result.error(); } } // namespace uprotocol::communication diff --git a/src/communication/Subscriber.cpp b/src/communication/Subscriber.cpp index e2275eac4..14ca07a3b 100644 --- a/src/communication/Subscriber.cpp +++ b/src/communication/Subscriber.cpp @@ -11,10 +11,12 @@ #include "up-cpp/communication/Subscriber.h" +#include + #include "up-cpp/datamodel/validator/UUri.h" namespace uprotocol::communication { -namespace UriValidator = uprotocol::datamodel::validator::uri; +namespace uri_validator = uprotocol::datamodel::validator::uri; [[nodiscard]] Subscriber::SubscriberOrStatus Subscriber::subscribe( std::shared_ptr transport, const v1::UUri& topic, @@ -25,27 +27,28 @@ namespace UriValidator = uprotocol::datamodel::validator::uri; } auto [source_ok, bad_source_reason] = - UriValidator::isValidSubscription(topic); + uri_validator::isValidSubscription(topic); if (!source_ok) { - throw UriValidator::InvalidUUri( + throw uri_validator::InvalidUUri( "Topic URI is not a valid topic subscription pattern | " + - std::string(UriValidator::message(*bad_source_reason))); + std::string(uri_validator::message(*bad_source_reason))); } auto handle = transport->registerListener(std::move(callback), topic); if (!handle) { - return uprotocol::utils::Unexpected(handle.error()); + return SubscriberOrStatus( + utils::Unexpected(handle.error())); } - return std::make_unique( + return SubscriberOrStatus(std::make_unique( std::forward>(transport), - std::forward(std::move(handle).value())); + std::forward(std::move(handle).value()))); } Subscriber::Subscriber(std::shared_ptr transport, ListenHandle&& subscription) - : transport_(transport), subscription_(std::move(subscription)) { + : transport_(std::move(transport)), subscription_(std::move(subscription)) { // Constructor body. Any additional setup can go here. if (!transport_) { throw transport::NullTransport("transport cannot be null"); diff --git a/src/datamodel/builder/Payload.cpp b/src/datamodel/builder/Payload.cpp index 83d0b8645..865ea9ff5 100644 --- a/src/datamodel/builder/Payload.cpp +++ b/src/datamodel/builder/Payload.cpp @@ -27,7 +27,7 @@ Payload::Payload(const std::string& value, const v1::UPayloadFormat format) { if (!UPayloadFormat_IsValid(format)) { throw std::out_of_range("Invalid String payload format"); } - payload_ = std::make_tuple(std::move(value), format); + payload_ = std::make_tuple(value, format); } // Move string constructor @@ -49,31 +49,26 @@ Payload::Payload(Serialized&& serialized) { // google::protobuf::Any constructor Payload::Payload(const google::protobuf::Any& any) { payload_ = std::make_tuple( - any.SerializeAsString(), - uprotocol::v1::UPayloadFormat::UPAYLOAD_FORMAT_PROTOBUF_WRAPPED_IN_ANY); + any.SerializeAsString(), + uprotocol::v1::UPayloadFormat::UPAYLOAD_FORMAT_PROTOBUF_WRAPPED_IN_ANY); } // Move constructor Payload::Payload(Payload&& other) noexcept - : payload_(std::move(other.payload_)), moved_(std::move(other.moved_)) {} + : payload_(std::move(other.payload_)), moved_(other.moved_) {} // Copy constructor -Payload::Payload(const Payload& other) - : payload_(other.payload_), moved_(other.moved_) {} +Payload::Payload(const Payload& other) = default; // Move assignment operator Payload& Payload::operator=(Payload&& other) noexcept { payload_ = std::move(other.payload_); - moved_ = std::move(other.moved_); + moved_ = other.moved_; return *this; } // Copy assignment operator -Payload& Payload::operator=(const Payload& other) { - payload_ = other.payload_; - moved_ = other.moved_; - return *this; -} +Payload& Payload::operator=(const Payload& other) = default; Payload::PayloadMoved::PayloadMoved(PayloadMoved&& other) noexcept : std::runtime_error(std::move(other)) {} @@ -86,15 +81,11 @@ Payload::PayloadMoved& Payload::PayloadMoved::operator=( } // PayloadMoved copy constructor -Payload::PayloadMoved::PayloadMoved(const PayloadMoved& other) - : std::runtime_error(other) {} +Payload::PayloadMoved::PayloadMoved(const PayloadMoved& other) = default; // PayloadMoved copy assignment operator Payload::PayloadMoved& Payload::PayloadMoved::operator=( - const PayloadMoved& other) { - std::runtime_error::operator=(other); - return *this; -} + const PayloadMoved& other) = default; // buildCopy method [[nodiscard]] const Payload::Serialized& Payload::buildCopy() const { diff --git a/src/datamodel/builder/UMessage.cpp b/src/datamodel/builder/UMessage.cpp index c2d433f25..7653a02c5 100644 --- a/src/datamodel/builder/UMessage.cpp +++ b/src/datamodel/builder/UMessage.cpp @@ -111,11 +111,11 @@ UMessageBuilder UMessageBuilder::response(v1::UUri&& sink, UMessageBuilder UMessageBuilder::response(const v1::UMessage& request) { v1::UUri sink = request.attributes().source(); - v1::UUID reqId = request.attributes().id(); + v1::UUID req_id = request.attributes().id(); v1::UPriority priority = request.attributes().priority(); v1::UUri method = request.attributes().sink(); - return UMessageBuilder::response(std::move(sink), std::move(reqId), + return UMessageBuilder::response(std::move(sink), std::move(req_id), priority, std::move(method)); } @@ -224,11 +224,11 @@ v1::UMessage UMessageBuilder::build(builder::Payload&& payload) const { return message; } -UMessageBuilder::UMessageBuilder(v1::UMessageType msgType, v1::UUri&& source, +UMessageBuilder::UMessageBuilder(v1::UMessageType msg_type, v1::UUri&& source, std::optional&& sink, std::optional&& request_id) : uuidBuilder_(UuidBuilder::getBuilder()) { - attributes_.set_type(msgType); + attributes_.set_type(msg_type); *attributes_.mutable_source() = std::move(source); diff --git a/src/datamodel/serializer/UUri.cpp b/src/datamodel/serializer/UUri.cpp index 78232fd7a..4b0351fe7 100644 --- a/src/datamodel/serializer/UUri.cpp +++ b/src/datamodel/serializer/UUri.cpp @@ -18,7 +18,10 @@ namespace uprotocol::datamodel::serializer::uri { std::string AsString::serialize(const v1::UUri& uri) { - using namespace uprotocol::datamodel::validator::uri; + using uprotocol::datamodel::validator::uri::InvalidUUri; + using uprotocol::datamodel::validator::uri::isLocal; + using uprotocol::datamodel::validator::uri::isValidFilter; + // isValidFilter is the most permissive of the validators auto [valid, reason] = isValidFilter(uri); if (!valid) { @@ -37,23 +40,24 @@ std::string AsString::serialize(const v1::UUri& uri) { return std::move(ss).str(); } -std::string_view extractSegment(std::string_view& uriView) { - constexpr std::string_view segment_separator = "/"; - const auto end = uriView.find(segment_separator); - if (end == uriView.npos) { +std::string_view extractSegment(std::string_view& uri_view) { + constexpr std::string_view SEGMENT_SEPARATOR = "/"; + const auto end = uri_view.find(SEGMENT_SEPARATOR); + if (end == std::string_view::npos) { throw std::invalid_argument("Could not extract segment from '" + - std::string(uriView) + + std::string(uri_view) + "' with separator '" + "/" + "'"); } - auto segment = uriView.substr(0, end); - uriView = uriView.substr(end + 1); + auto segment = uri_view.substr(0, end); + uri_view = uri_view.substr(end + 1); return segment; } uint32_t segmentToUint32(const std::string_view& segment) { uint32_t value = 0; + constexpr auto HEX_BASE = 16; auto [end, ec] = std::from_chars( - segment.data(), segment.data() + segment.size(), value, 16); + segment.data(), segment.data() + segment.size(), value, HEX_BASE); const bool convert_ok = (ec == std::errc{}) && (end == segment.data() + segment.size()); if (!convert_ok) { @@ -63,51 +67,52 @@ uint32_t segmentToUint32(const std::string_view& segment) { return value; } -uprotocol::v1::UUri AsString::deserialize(const std::string& uriAsString) { - if (uriAsString.empty()) { +uprotocol::v1::UUri AsString::deserialize(const std::string& uri_as_string) { + if (uri_as_string.empty()) { throw std::invalid_argument("Cannot deserialize empty string"); } - constexpr std::string_view schema_prefix = "up://"; - constexpr std::string_view remote_prefix = "//"; - constexpr std::string_view segment_separator = "/"; + constexpr std::string_view SCHEMA_PREFIX = "up://"; + constexpr std::string_view REMOTE_PREFIX = "//"; + constexpr std::string_view SEGMENT_SEPARATOR = "/"; // Operating on a string view to avoid copies and reallocations - std::string_view uriView(uriAsString); + std::string_view uri_view(uri_as_string); // Extract and convert the rest of the URI string v1::UUri uri; // Verify start and extract Authority, if present // With up:// schema - if (uriView.substr(0, schema_prefix.size()) == schema_prefix) { + if (uri_view.substr(0, SCHEMA_PREFIX.size()) == SCHEMA_PREFIX) { // Advance past the prefix - uriView = uriView.substr(schema_prefix.size()); - uri.set_authority_name(std::string(extractSegment(uriView))); + uri_view = uri_view.substr(SCHEMA_PREFIX.size()); + uri.set_authority_name(std::string(extractSegment(uri_view))); // with // remote prefix - } else if (uriView.substr(0, remote_prefix.size()) == remote_prefix) { + } else if (uri_view.substr(0, REMOTE_PREFIX.size()) == REMOTE_PREFIX) { // Advance past the prefix - uriView = uriView.substr(remote_prefix.size()); - uri.set_authority_name(std::string(extractSegment(uriView))); + uri_view = uri_view.substr(REMOTE_PREFIX.size()); + uri.set_authority_name(std::string(extractSegment(uri_view))); // with / local prefix - } else if (uriView.substr(0, segment_separator.size()) == - segment_separator) { + } else if (uri_view.substr(0, SEGMENT_SEPARATOR.size()) == + SEGMENT_SEPARATOR) { // Advance past the prefix - uriView = uriView.substr(segment_separator.size()); + uri_view = uri_view.substr(SEGMENT_SEPARATOR.size()); // Missing required prefix } else { throw std::invalid_argument( "Did not find expected URI start in string: '" + - std::string(uriView) + "'"); + std::string(uri_view) + "'"); } - uri.set_ue_id(segmentToUint32(extractSegment(uriView))); - uri.set_ue_version_major(segmentToUint32(extractSegment(uriView))); - uri.set_resource_id(segmentToUint32(uriView)); + uri.set_ue_id(segmentToUint32(extractSegment(uri_view))); + uri.set_ue_version_major(segmentToUint32(extractSegment(uri_view))); + uri.set_resource_id(segmentToUint32(uri_view)); { - using namespace uprotocol::datamodel::validator::uri; + using uprotocol::datamodel::validator::uri::InvalidUUri; + using uprotocol::datamodel::validator::uri::isValidFilter; // isValidFilter is the most permissive of the validators auto [valid, reason] = isValidFilter(uri); if (!valid) { diff --git a/src/datamodel/serializer/Uuid.cpp b/src/datamodel/serializer/Uuid.cpp index 781a781ee..26c0f0dbf 100644 --- a/src/datamodel/serializer/Uuid.cpp +++ b/src/datamodel/serializer/Uuid.cpp @@ -21,15 +21,15 @@ namespace { constexpr uint64_t RANDOM_B_SHIFT = 48; -constexpr size_t MSB_HIGH_ = 0; -constexpr size_t MSB_LOW__ = 4; -constexpr size_t LSB_HIGH_ = 8; -constexpr size_t LSB_LOW__ = 12; +constexpr size_t MSB_HIGH = 0; +constexpr size_t MSB_LOW = 4; +constexpr size_t LSB_HIGH = 8; +constexpr size_t LSB_LOW = 12; } // namespace namespace uprotocol::datamodel::serializer::uuid { -std::string AsString::serialize(const uprotocol::v1::UUID uuid) { +std::string AsString::serialize(const uprotocol::v1::UUID& uuid) { // Extracting the parts of the UUIDv7 uint64_t unix_ts_ms = (uuid.msb() >> UUID_TIMESTAMP_SHIFT) & UUID_TIMESTAMP_MASK; @@ -40,19 +40,20 @@ std::string AsString::serialize(const uprotocol::v1::UUID uuid) { // Formatting the UUIDv8 in the traditional format std::stringstream ss; - ss << std::hex << std::setfill('0') << std::setw(8) - << ((unix_ts_ms >> 16) & MASK_32_BITS) // First 32 bits of timestamp - << "-" << std::setw(4) + ss << std::hex << std::setfill('0') << std::setw(LEN_32_BITS_IN_HEX) + << ((unix_ts_ms >> LEN_LSB_IN_HEX * LEN_HEX_TO_BIT) & + MASK_32_BITS) // First 32 bits of timestamp + << "-" << std::setw(LEN_16_BITS_IN_HEX) << ((unix_ts_ms)&MASK_16_BITS) // Next 16 bits of timestamp i.e. last 16 // bits of ts - << "-" << std::setw(4) + << "-" << std::setw(LEN_16_BITS_IN_HEX) << (((ver & UUID_VERSION_MASK) << UUID_VERSION_SHIFT) | (rand_a & UUID_RANDOM_A_MASK)) // Last 16 bits of timestamp and version - << "-" << std::setw(4) - << (((var & UUID_VARIANT_MASK) << 14) | + << "-" << std::setw(LEN_16_BITS_IN_HEX) + << (((var & UUID_VARIANT_MASK) << (LEN_HEX_TO_BIT * 3 + 2)) | ((rand_b >> RANDOM_B_SHIFT) & MASK_14_BITS)) // Variant and randb - << "-" << std::setw(12) + << "-" << std::setw(LEN_48_BITS_IN_HEX) << (rand_b & UUID_TIMESTAMP_MASK); // Random number return std::move(ss).str(); @@ -72,8 +73,9 @@ uprotocol::v1::UUID AsString::deserialize(const std::string& str) { // Please check UP-spec for UUID formatting: // https://github.com/eclipse-uprotocol/up-spec/blob/main/basics/uuid.adoc - if (str.length() != 36 || str[8] != '-' || str[13] != '-' || - str[18] != '-' || str[23] != '-') { + if (str.length() != TOTAL_UUID_LENGTH || str[POS_FIRST_SEPARATOR] != '-' || + str[POS_SECOND_SEPARATOR] != '-' || str[POS_THIRD_SEPARATOR] != '-' || + str[POS_FOURTH_SEPARATOR] != '-') { throw std::invalid_argument("Invalid UUID string format"); } @@ -86,45 +88,54 @@ uprotocol::v1::UUID AsString::deserialize(const std::string& str) { try { // Extract the parts from the UUID string - unix_ts_ms = std::stoull(str.substr(0, 8), nullptr, HEX_BASE) << 16; - unix_ts_ms |= std::stoull(str.substr(9, 4), nullptr, HEX_BASE); + unix_ts_ms = + std::stoull(str.substr(0, LEN_MSB_IN_HEX), nullptr, HEX_BASE) + << LEN_LSB_IN_HEX * LEN_HEX_TO_BIT; + unix_ts_ms |= + std::stoull(str.substr(POS_FIRST_SEPARATOR + 1, LEN_LSB_IN_HEX), + nullptr, HEX_BASE); uint16_t msb_low = static_cast( - std::stoul(str.substr(14, 4), nullptr, HEX_BASE)); - ver = (msb_low >> 12) & UUID_VERSION_MASK; + std::stoul(str.substr(POS_SECOND_SEPARATOR + 1, LEN_VCANT_IN_HEX), + nullptr, HEX_BASE)); + ver = (msb_low >> LEN_HEX_TO_BIT * 3) & UUID_VERSION_MASK; rand_a = msb_low & UUID_RANDOM_A_MASK; uint16_t var_randb = static_cast( - std::stoul(str.substr(19, 4), nullptr, HEX_BASE)); - var = (var_randb >> 14) & UUID_VARIANT_MASK; + std::stoul(str.substr(POS_THIRD_SEPARATOR + 1, LEN_VARR_IN_HEX), + nullptr, HEX_BASE)); + var = (var_randb >> (LEN_HEX_TO_BIT * 3 + 2)) & UUID_VARIANT_MASK; rand_b = static_cast(var_randb & MASK_14_BITS) << RANDOM_B_SHIFT; - rand_b |= std::stoull(str.substr(24), nullptr, HEX_BASE); + rand_b |= std::stoull(str.substr(POS_FOURTH_SEPARATOR + 1), nullptr, + HEX_BASE); } catch (const std::exception& e) { throw std::invalid_argument("Invalid UUID string format"); } // Reconstruct the UUID uuid.set_msb((unix_ts_ms << UUID_TIMESTAMP_SHIFT) | - (ver << UUID_VERSION_SHIFT) | rand_a); + ((static_cast(ver) << UUID_VERSION_SHIFT)) | rand_a); uuid.set_lsb((static_cast(var) << UUID_VARIANT_SHIFT) | rand_b); return uuid; } // Serialization function -std::vector AsBytes::serialize(const v1::UUID uuid) { +std::vector AsBytes::serialize(const v1::UUID& uuid) { std::vector bytes(UUID_BYTE_SIZE); - uint32_t msb_high = htonl(static_cast(uuid.msb() >> 32)); + uint32_t msb_high = + htonl(static_cast(uuid.msb() >> LEN_UINT64_IN_BIT / 2)); uint32_t msb_low = htonl(static_cast(uuid.msb() & MASK_32_BITS)); - uint32_t lsb_high = htonl(static_cast(uuid.lsb() >> 32)); + uint32_t lsb_high = + htonl(static_cast(uuid.lsb() >> LEN_UINT64_IN_BIT / 2)); uint32_t lsb_low = htonl(static_cast(uuid.lsb() & MASK_32_BITS)); - std::memcpy(&bytes[MSB_HIGH_], &msb_high, UUID_PART_SIZE); - std::memcpy(&bytes[MSB_LOW__], &msb_low, UUID_PART_SIZE); - std::memcpy(&bytes[LSB_HIGH_], &lsb_high, UUID_PART_SIZE); - std::memcpy(&bytes[LSB_LOW__], &lsb_low, UUID_PART_SIZE); + std::memcpy(&bytes[MSB_HIGH], &msb_high, UUID_PART_SIZE); + std::memcpy(&bytes[MSB_LOW], &msb_low, UUID_PART_SIZE); + std::memcpy(&bytes[LSB_HIGH], &lsb_high, UUID_PART_SIZE); + std::memcpy(&bytes[LSB_LOW], &lsb_low, UUID_PART_SIZE); return bytes; } @@ -134,17 +145,22 @@ v1::UUID AsBytes::deserialize(const std::vector& bytes) { throw std::invalid_argument("Invalid UUID byte array size"); } - uint32_t msb_high, msb_low, lsb_high, lsb_low; + uint32_t msb_high = 0; + uint32_t msb_low = 0; + uint32_t lsb_high = 0; + uint32_t lsb_low = 0; - std::memcpy(&msb_high, &bytes[MSB_HIGH_], UUID_PART_SIZE); - std::memcpy(&msb_low, &bytes[MSB_LOW__], UUID_PART_SIZE); - std::memcpy(&lsb_high, &bytes[LSB_HIGH_], UUID_PART_SIZE); - std::memcpy(&lsb_low, &bytes[LSB_LOW__], UUID_PART_SIZE); + std::memcpy(&msb_high, &bytes[MSB_HIGH], UUID_PART_SIZE); + std::memcpy(&msb_low, &bytes[MSB_LOW], UUID_PART_SIZE); + std::memcpy(&lsb_high, &bytes[LSB_HIGH], UUID_PART_SIZE); + std::memcpy(&lsb_low, &bytes[LSB_LOW], UUID_PART_SIZE); uint64_t msb = - (static_cast(ntohl(msb_high)) << 32) | ntohl(msb_low); + (static_cast(ntohl(msb_high)) << LEN_UINT64_IN_BIT / 2) | + ntohl(msb_low); uint64_t lsb = - (static_cast(ntohl(lsb_high)) << 32) | ntohl(lsb_low); + (static_cast(ntohl(lsb_high)) << LEN_UINT64_IN_BIT / 2) | + ntohl(lsb_low); v1::UUID uuid; uuid.set_msb(msb); diff --git a/src/datamodel/validator/UMessage.cpp b/src/datamodel/validator/UMessage.cpp index d8eab414b..6a9db80c7 100644 --- a/src/datamodel/validator/UMessage.cpp +++ b/src/datamodel/validator/UMessage.cpp @@ -18,8 +18,8 @@ namespace uprotocol::datamodel::validator::message { -using namespace uprotocol::v1; -using namespace uprotocol::datamodel::validator; +using uprotocol::v1::UPRIORITY_CS4; +// using uprotocol::datamodel::validator; std::string_view message(Reason reason) { switch (reason) { diff --git a/src/datamodel/validator/UUri.cpp b/src/datamodel/validator/UUri.cpp index cdd7d1dfb..f623b0f66 100644 --- a/src/datamodel/validator/UUri.cpp +++ b/src/datamodel/validator/UUri.cpp @@ -14,8 +14,13 @@ namespace { constexpr size_t AUTHORITY_SPEC_MAX_LENGTH = 128; +// TODO(max) try to find a better name +constexpr auto START_OF_TOPICS = 0x8000; +constexpr auto MAX_RESOURCE_ID = 0xFFFF; + +using uprotocol::datamodel::validator::uri::Reason; +using uprotocol::datamodel::validator::uri::ValidationResult; -using namespace uprotocol::datamodel::validator::uri; ValidationResult uriCommonValidChecks(const uprotocol::v1::UUri& uuri) { if (uuri.ue_version_major() == 0) { return {false, Reason::RESERVED_VERSION}; @@ -68,19 +73,25 @@ std::string_view message(Reason reason) { } bool uses_wildcards(const v1::UUri& uuri) { - if (uuri.authority_name().find("*") != std::string::npos) { + constexpr auto LOWER_8_BIT_MASK = 0xFF; + constexpr auto LOWER_16_BIT_MASK = 0xFFFF; + constexpr auto UPPER_16_BIT_MASK = 0xFFFF0000; + + if (uuri.authority_name().find_first_of('*') != std::string::npos) { return true; } - if ((uuri.ue_id() & 0xFFFF) == 0xFFFF) { // service ID + if ((uuri.ue_id() & LOWER_16_BIT_MASK) == + LOWER_16_BIT_MASK) { // service ID return true; } - if ((uuri.ue_id() & 0xFFFF0000) == 0xFFFF0000) { // service instance ID + if ((uuri.ue_id() & UPPER_16_BIT_MASK) == + UPPER_16_BIT_MASK) { // service instance ID return true; } - if (uuri.ue_version_major() == 0xFF) { + if (uuri.ue_version_major() == LOWER_8_BIT_MASK) { return true; } - if (uuri.resource_id() == 0xFFFF) { + if (uuri.resource_id() == LOWER_16_BIT_MASK) { return true; } return false; @@ -139,7 +150,7 @@ ValidationResult isValidRpcMethod(const v1::UUri& uuri) { } // check resource ID [0x0001, 0x7FFF] - if (uuri.resource_id() == 0 || uuri.resource_id() > 0x7FFF) { + if (uuri.resource_id() == 0 || uuri.resource_id() >= START_OF_TOPICS) { return {false, Reason::BAD_RESOURCE_ID}; } @@ -176,7 +187,8 @@ ValidationResult isValidPublishTopic(const v1::UUri& uuri) { return {false, Reason::DISALLOWED_WILDCARD}; } - if ((uuri.resource_id() < 0x8000) || (uuri.resource_id() > 0xFFFF)) { + if ((uuri.resource_id() < START_OF_TOPICS) || + (uuri.resource_id() > MAX_RESOURCE_ID)) { return {false, Reason::BAD_RESOURCE_ID}; } @@ -189,7 +201,8 @@ ValidationResult isValidNotificationSource(const v1::UUri& uuri) { return {false, Reason::DISALLOWED_WILDCARD}; } - if ((uuri.resource_id() < 0x8000) || (uuri.resource_id() > 0xFFFF)) { + if ((uuri.resource_id() < START_OF_TOPICS) || + (uuri.resource_id() > MAX_RESOURCE_ID)) { return {false, Reason::BAD_RESOURCE_ID}; } @@ -210,7 +223,8 @@ ValidationResult isValidNotificationSink(const v1::UUri& uuri) { } ValidationResult isValidSubscription(const v1::UUri& uuri) { - if (uuri.resource_id() < 0x8000 || uuri.resource_id() > 0xFFFF) { + if (uuri.resource_id() < START_OF_TOPICS || + uuri.resource_id() > MAX_RESOURCE_ID) { return {false, Reason::BAD_RESOURCE_ID}; } diff --git a/src/datamodel/validator/Uuid.cpp b/src/datamodel/validator/Uuid.cpp index 26e4f7b15..2530045cb 100644 --- a/src/datamodel/validator/Uuid.cpp +++ b/src/datamodel/validator/Uuid.cpp @@ -15,17 +15,16 @@ using namespace std::chrono_literals; -namespace { +namespace uprotocol::datamodel { -using namespace uprotocol::datamodel; -using milliseconds = std::chrono::milliseconds; +using Milliseconds = std::chrono::milliseconds; std::chrono::system_clock::time_point getUuidTimestamp( const uprotocol::v1::UUID& uuid) { uint64_t msb = uuid.msb(); uint64_t timestamp = (msb >> UUID_TIMESTAMP_SHIFT) & UUID_TIMESTAMP_MASK; - return std::chrono::system_clock::time_point(milliseconds(timestamp)); + return std::chrono::system_clock::time_point(Milliseconds(timestamp)); } uint8_t internalGetVersion(const uprotocol::v1::UUID& uuid) { @@ -36,7 +35,7 @@ uint8_t internalGetVariant(const uprotocol::v1::UUID& uuid) { return (uuid.lsb() >> UUID_VARIANT_SHIFT) & UUID_VARIANT_MASK; } -} // namespace +} // namespace uprotocol::datamodel namespace uprotocol::datamodel::validator::uuid { @@ -55,7 +54,7 @@ std::string_view message(Reason reason) { } } -ValidationResult isUuid(const uprotocol::v1::UUID uuid) { +ValidationResult isUuid(const uprotocol::v1::UUID& uuid) { uint8_t version = internalGetVersion(uuid); if (version != UUID_VERSION_7) { return {false, Reason::WRONG_VERSION}; @@ -76,7 +75,7 @@ ValidationResult isUuid(const uprotocol::v1::UUID uuid) { return {true, std::nullopt}; } -ValidationResult isExpired(const uprotocol::v1::UUID uuid, +ValidationResult isExpired(const uprotocol::v1::UUID& uuid, std::chrono::milliseconds ttl) { auto [valid, reason] = isUuid(uuid); if (!valid) { @@ -93,7 +92,7 @@ ValidationResult isExpired(const uprotocol::v1::UUID uuid, return {false, std::nullopt}; } -uint8_t getVersion(const uprotocol::v1::UUID uuid) { +uint8_t getVersion(const uprotocol::v1::UUID& uuid) { auto [valid, reason] = isUuid(uuid); if (!valid) { throw InvalidUuid(message(reason.value())); @@ -101,7 +100,7 @@ uint8_t getVersion(const uprotocol::v1::UUID uuid) { return internalGetVersion(uuid); } -uint8_t getVariant(const uprotocol::v1::UUID uuid) { +uint8_t getVariant(const uprotocol::v1::UUID& uuid) { auto [valid, reason] = isUuid(uuid); if (!valid) { throw InvalidUuid(message(reason.value())); @@ -109,7 +108,7 @@ uint8_t getVariant(const uprotocol::v1::UUID uuid) { return internalGetVariant(uuid); } -std::chrono::system_clock::time_point getTime(const uprotocol::v1::UUID uuid) { +std::chrono::system_clock::time_point getTime(const uprotocol::v1::UUID& uuid) { auto [valid, reason] = isUuid(uuid); if (!valid) { throw InvalidUuid(message(reason.value())); @@ -117,7 +116,7 @@ std::chrono::system_clock::time_point getTime(const uprotocol::v1::UUID uuid) { return getUuidTimestamp(uuid); } -std::chrono::milliseconds getElapsedTime(const uprotocol::v1::UUID uuid) { +std::chrono::milliseconds getElapsedTime(const uprotocol::v1::UUID& uuid) { auto [valid, reason] = isUuid(uuid); if (!valid) { throw InvalidUuid(message(reason.value())); @@ -126,10 +125,10 @@ std::chrono::milliseconds getElapsedTime(const uprotocol::v1::UUID uuid) { auto current_time = std::chrono::system_clock::now(); auto uuid_time = getTime(uuid); - return std::chrono::duration_cast(current_time - uuid_time); + return std::chrono::duration_cast(current_time - uuid_time); } -std::chrono::milliseconds getRemainingTime(const uprotocol::v1::UUID uuid, +std::chrono::milliseconds getRemainingTime(const uprotocol::v1::UUID& uuid, std::chrono::milliseconds ttl) { auto elapsed_time = getElapsedTime(uuid); return std::max(ttl - elapsed_time, 0ms); diff --git a/src/transport/UTransport.cpp b/src/transport/UTransport.cpp index 0904504ae..2f51eb823 100644 --- a/src/transport/UTransport.cpp +++ b/src/transport/UTransport.cpp @@ -11,30 +11,33 @@ #include "up-cpp/transport/UTransport.h" +#include + #include "up-cpp/datamodel/validator/UMessage.h" #include "up-cpp/datamodel/validator/UUri.h" #include "up-cpp/utils/Expected.h" namespace uprotocol::transport { -namespace UriValidator = uprotocol::datamodel::validator::uri; -namespace MessageValidator = uprotocol::datamodel::validator::message; +namespace uri_validator = uprotocol::datamodel::validator::uri; +namespace message_validator = uprotocol::datamodel::validator::message; -UTransport::UTransport(const v1::UUri& entity_uri) : entity_uri_(entity_uri) { - auto [uri_ok, reason] = UriValidator::isValidDefaultEntity(entity_uri_); +UTransport::UTransport(v1::UUri entity_uri) + : entity_uri_(std::move(entity_uri)) { + auto [uri_ok, reason] = uri_validator::isValidDefaultEntity(entity_uri_); if (!uri_ok) { - throw UriValidator::InvalidUUri( + throw uri_validator::InvalidUUri( "Transport's entity URI is not a valid URI | " + - std::string(UriValidator::message(*reason))); + std::string(uri_validator::message(*reason))); } } v1::UStatus UTransport::send(const v1::UMessage& message) { - auto [msgOk, reason] = MessageValidator::isValid(message); + auto [msgOk, reason] = message_validator::isValid(message); if (!msgOk) { - throw MessageValidator::InvalidUMessage( + throw message_validator::InvalidUMessage( "Invalid UMessage | " + - std::string(MessageValidator::message(*reason))); + std::string(message_validator::message(*reason))); } return sendImpl(message); @@ -56,27 +59,27 @@ UTransport::HandleOrStatus UTransport::registerListener( // Handle the special case of publish-like messages where only a source // address is provided. auto [source_ok, bad_source_reason] = - UriValidator::isValidSubscription(source_filter); + uri_validator::isValidSubscription(source_filter); if (!source_ok) { - throw UriValidator::InvalidUUri( + throw uri_validator::InvalidUUri( "source_filter is not a valid URI | " + - std::string(UriValidator::message(*bad_source_reason))); + std::string(uri_validator::message(*bad_source_reason))); } } else { auto [source_ok, bad_source_reason] = - UriValidator::isValidFilter(source_filter); + uri_validator::isValidFilter(source_filter); if (!source_ok) { - throw UriValidator::InvalidUUri( + throw uri_validator::InvalidUUri( "source_filter is not a valid URI | " + - std::string(UriValidator::message(*bad_source_reason))); + std::string(uri_validator::message(*bad_source_reason))); } auto [sink_ok, bad_sink_reason] = - UriValidator::isValidFilter(*sink_filter); + uri_validator::isValidFilter(*sink_filter); if (!sink_ok) { - throw UriValidator::InvalidUUri( + throw uri_validator::InvalidUUri( "sink_filter is not a valid URI | " + - std::string(UriValidator::message(*bad_sink_reason))); + std::string(uri_validator::message(*bad_sink_reason))); } } @@ -87,15 +90,14 @@ UTransport::HandleOrStatus UTransport::registerListener( std::move(callable), source_filter, std::move(sink_filter)); if (status.code() == v1::UCode::OK) { - return std::move(handle); - } else { - return utils::Unexpected(std::move(status)); + return HandleOrStatus(std::move(handle)); } + return HandleOrStatus(utils::Unexpected(status)); } // NOTE: deprecated utils::Expected -UTransport::registerListener(const v1::UUri& sink_filter, +UTransport::registerListener(const v1::UUri& sink_or_topic_filter, ListenCallback&& listener, std::optional&& source_filter) { if (!source_filter) { @@ -104,20 +106,19 @@ UTransport::registerListener(const v1::UUri& sink_filter, // as meaning that the sink_filter is a topic to subscribe to. This // will then pass the sink_filter as a source filter to the updated // registerListener(), which is then called with sink_filter unset. - return registerListener(std::move(listener), sink_filter, + return registerListener(std::move(listener), sink_or_topic_filter, std::move(source_filter)); - } else { - v1::UUri sink_filter_copy = sink_filter; - return registerListener(std::move(listener), *source_filter, - std::move(sink_filter_copy)); } + v1::UUri sink_filter_copy = sink_or_topic_filter; + return registerListener(std::move(listener), *source_filter, + std::move(sink_filter_copy)); } const v1::UUri& UTransport::getEntityUri() const { return entity_uri_; } const v1::UUri& UTransport::getDefaultSource() const { return getEntityUri(); } -void UTransport::cleanupListener(CallableConn listener) { +void UTransport::cleanupListener(const CallableConn& listener) { static_cast(listener); } diff --git a/src/utils/ProtoConverter.cpp b/src/utils/ProtoConverter.cpp index 61460ba10..c4159ee9f 100644 --- a/src/utils/ProtoConverter.cpp +++ b/src/utils/ProtoConverter.cpp @@ -3,13 +3,14 @@ namespace uprotocol::utils { google::protobuf::Timestamp ProtoConverter::ConvertToProtoTimestamp( const std::chrono::system_clock::time_point& tp) { + constexpr auto NANOSECONDS_PER_SECOND = 1000000000LL; google::protobuf::Timestamp timestamp; auto duration = tp.time_since_epoch(); auto seconds = std::chrono::duration_cast(duration).count(); auto nanoseconds = std::chrono::duration_cast(duration).count() % - 1000000000ll; + NANOSECONDS_PER_SECOND; timestamp.set_seconds(seconds); timestamp.set_nanos(static_cast(nanoseconds)); diff --git a/test/coverage/datamodel/PayloadBuilderTest.cpp b/test/coverage/datamodel/PayloadBuilderTest.cpp index c3ffac21f..ac8d46da6 100644 --- a/test/coverage/datamodel/PayloadBuilderTest.cpp +++ b/test/coverage/datamodel/PayloadBuilderTest.cpp @@ -130,9 +130,8 @@ TEST_F(PayloadTest, CreateSerializedProtobufPayloadAndMoveTwiceExceptionTest) { uprotocol::v1::UPayloadFormat::UPAYLOAD_FORMAT_PROTOBUF); EXPECT_EQ(payloadData, uriObject.SerializeAsString()); - EXPECT_THROW({ - auto _ = std::move(payload).buildMove(); - }, Payload::PayloadMoved); + EXPECT_THROW({ auto _ = std::move(payload).buildMove(); }, + Payload::PayloadMoved); } // Create serialized protobuf payload. Call build after move. diff --git a/test/include/UTransportMock.h b/test/include/UTransportMock.h index 29129e6c3..0121dac55 100644 --- a/test/include/UTransportMock.h +++ b/test/include/UTransportMock.h @@ -73,7 +73,7 @@ class UTransportMock : public uprotocol::transport::UTransport { return registerListener_status_; } - void cleanupListener(CallableConn listener) override { + void cleanupListener(const CallableConn& listener) override { cleanup_listener_ = listener; } };