From b640f90c65144a19ae6ea5c5db84fb4c96ea6680 Mon Sep 17 00:00:00 2001 From: maximiliantoe Date: Thu, 6 Mar 2025 08:32:05 +0100 Subject: [PATCH 1/4] fixed clang-tidy linting warnings and error in include/* and src/* changed const to const ref in validator/uuid removed more linter warnings and errors fixed more linting errors linting fixes and pinned clang-tidy 13 in ci.yml fixed more liting errors fixed most warnings/errors except magic numbers and closure fixed linting errors in RpcClient.cpp fixed all errors, only warnings remainings test pipeline fixed all errors during build fixed signature mismatch in header file save builds and all tests pass replaced some magic numbers fixed magic numbers in uuid.cpp replaced all magic numbers modified PendingRequest in RpcClient.cpp with public constructor deleted unsued code removed unused setters --- .github/workflows/ci.yml | 1 + .../up-cpp/client/usubscription/v3/Consumer.h | 22 +-- .../up-cpp/communication/NotificationSink.h | 5 +- include/up-cpp/communication/RpcClient.h | 4 +- include/up-cpp/communication/RpcServer.h | 2 +- include/up-cpp/datamodel/builder/Payload.h | 14 +- include/up-cpp/datamodel/builder/UMessage.h | 4 +- include/up-cpp/datamodel/builder/Uuid.h | 2 +- .../datamodel/constants/UuidConstants.h | 26 ++++ include/up-cpp/datamodel/serializer/Uuid.h | 4 +- include/up-cpp/datamodel/validator/UUri.h | 2 +- include/up-cpp/datamodel/validator/Uuid.h | 14 +- include/up-cpp/transport/UTransport.h | 13 +- include/up-cpp/utils/CallbackConnection.h | 20 +-- include/up-cpp/utils/CyclicQueue.h | 2 +- include/up-cpp/utils/Expected.h | 45 +++--- include/up-cpp/utils/IpAddress.h | 6 +- include/up-cpp/utils/ProtoConverter.h | 12 +- include/up-cpp/utils/ThreadPool.h | 2 +- include/up-cpp/utils/base64.h | 4 +- lint/clang-tidy.sh | 10 +- src/client/usubscription/v3/Consumer.cpp | 61 ++++---- src/communication/NotificationSink.cpp | 12 +- src/communication/NotificationSource.cpp | 7 +- src/communication/Publisher.cpp | 4 +- src/communication/RpcClient.cpp | 136 ++++++++++-------- src/communication/RpcServer.cpp | 31 ++-- src/communication/Subscriber.cpp | 18 +-- src/datamodel/builder/Payload.cpp | 24 ++-- src/datamodel/builder/UMessage.cpp | 8 +- src/datamodel/serializer/UUri.cpp | 67 +++++---- src/datamodel/serializer/Uuid.cpp | 78 +++++----- src/datamodel/validator/UMessage.cpp | 4 +- src/datamodel/validator/UUri.cpp | 29 ++-- src/datamodel/validator/Uuid.cpp | 25 ++-- src/transport/UTransport.cpp | 56 ++++---- src/utils/ProtoConverter.cpp | 3 +- .../client/usubscription/v3/ConsumerTest.cpp | 22 +-- .../communication/NotificationSinkTest.cpp | 2 +- .../communication/NotificationSourceTest.cpp | 40 +++--- test/coverage/datamodel/UUriValidatorTest.cpp | 14 +- test/coverage/datamodel/UuidBuilderTest.cpp | 2 +- .../coverage/utils/CallbackConnectionTest.cpp | 2 +- test/coverage/utils/ExpectedTest.cpp | 20 +-- test/include/UTransportMock.h | 13 +- 45 files changed, 482 insertions(+), 410 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1d038bccb..e54857bab 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -431,6 +431,7 @@ jobs: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} with: repo-root: up-cpp + version: 13 ignore: 'test' style: 'file' # read .clang-format for configuration tidy-checks: '' # Read .clang-tidy for configuration diff --git a/include/up-cpp/client/usubscription/v3/Consumer.h b/include/up-cpp/client/usubscription/v3/Consumer.h index 8f4964d63..5a7eb5e2d 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::uSubscription; +using uprotocol::core::usubscription::v3::Update; +using uprotocol::core::usubscription::v3::UnsubscribeRequest; +using uprotocol::core::usubscription::v3::SubscriptionRequest; /** * @struct ConsumerOptions @@ -54,7 +56,7 @@ struct ConsumerOptions { /// 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 { +struct USubscriptionUUriBuilder { private: /// URI for the uSubscription service v1::UUri uri_; @@ -63,7 +65,7 @@ struct uSubscriptionUUriBuilder { public: /// @brief Constructor for uSubscriptionUUriBuilder. - uSubscriptionUUriBuilder() { + USubscriptionUUriBuilder() { // Get the service descriptor const google::protobuf::ServiceDescriptor* service = uSubscription::descriptor(); @@ -128,7 +130,7 @@ struct Consumer { /// @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); @@ -156,7 +158,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 +171,7 @@ struct Consumer { ConsumerOptions consumer_options_; // URI info about the uSubscription service - uSubscriptionUUriBuilder uSubscriptionUUriBuilder_; + USubscriptionUUriBuilder uSubscriptionUUriBuilder_; // Subscription updates std::unique_ptr noficationSinkHandle_; @@ -214,4 +216,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..2b90d6378 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..42de21e1d 100644 --- a/include/up-cpp/communication/RpcServer.h +++ b/include/up-cpp/communication/RpcServer.h @@ -90,7 +90,7 @@ 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, + explicit RpcServer(std::shared_ptr transport, std::optional format = {}, std::optional ttl = {}); diff --git a/include/up-cpp/datamodel/builder/Payload.h b/include/up-cpp/datamodel/builder/Payload.h index 1fdd49fed..1f3a91a9a 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. @@ -96,7 +96,7 @@ struct Payload { /// /// @throws std::out_of_range If format is not valid for v1::UPayloadFormat Payload(const std::vector& value_bytes, - const v1::UPayloadFormat format); + v1::UPayloadFormat format); /// @brief Creates a Payload builder with a provided pre-serialized data. /// @@ -107,7 +107,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 +120,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..a291f497e 100644 --- a/include/up-cpp/datamodel/constants/UuidConstants.h +++ b/include/up-cpp/datamodel/constants/UuidConstants.h @@ -38,6 +38,32 @@ 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..d5f6fd17b 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..b5d58508f 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,7 @@ 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 +230,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 +242,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 +257,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 +333,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 +351,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 +364,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 +390,7 @@ 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 +440,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..153903712 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,15 @@ 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 +80,50 @@ class Expected { } constexpr const T& value() const& { - if (!has_value()) + if (!has_value()) { throw BadExpectedAccess( - "Attempt to access value() when unexpected."); + "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."); + "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."); + "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."); + "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."); + "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."); + "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..81fd4e13d 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,11 @@ #include namespace uprotocol::utils { -using namespace uprotocol::core::usubscription::v3; +using uprotocol::core::usubscription::v3::SubscriberInfo; +using uprotocol::core::usubscription::v3::SubscribeAttributes; +using uprotocol::core::usubscription::v3::SubscriptionRequest; +using uprotocol::core::usubscription::v3::UnsubscribeRequest; + struct ProtoConverter { /// @brief Converts std::chrono::time_point to google::protobuf::Timestamp @@ -52,4 +56,4 @@ struct ProtoConverter { const v1::UUri& subscription_topic); }; }; // namespace uprotocol::utils -#endif // PROTO_CONVERTER_H +#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-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..4c9775b1e 100644 --- a/src/client/usubscription/v3/Consumer.cpp +++ b/src/client/usubscription/v3/Consumer.cpp @@ -11,22 +11,23 @@ #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 +41,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() { @@ -67,24 +66,23 @@ v1::UStatus Consumer::createNotificationSink() { auto result = communication::NotificationSink::create( transport_, std::move(notification_sink_callback), - std::move(notification_topic)); + 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; } @@ -97,10 +95,10 @@ v1::UStatus Consumer::subscribe( 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..9d6aa1647 100644 --- a/src/communication/NotificationSink.cpp +++ b/src/communication/NotificationSink.cpp @@ -19,7 +19,7 @@ namespace uprotocol::communication { namespace UriValidator = datamodel::validator::uri; NotificationSink::SinkOrStatus NotificationSink::create( - std::shared_ptr transport, ListenCallback&& callback, + const std::shared_ptr& transport, ListenCallback&& callback, const uprotocol::v1::UUri& source_filter) { // Standard check - transport pointer cannot be null if (!transport) { @@ -40,17 +40,17 @@ 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..d9459cf1e 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..ab0c6dbb7 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,26 @@ 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 +105,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 +140,10 @@ RpcClient::InvokeHandle RpcClient::invokeMethod(v1::UMessage&& request, // attempt to call the callback succeeds. auto callback_once = std::make_shared(); - auto [callback_handle, callable] = + 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. @@ -147,7 +165,7 @@ RpcClient::InvokeHandle RpcClient::invokeMethod(v1::UMessage&& request, status.set_message("Received response with !OK commstatus"); std::call_once(*callback_once, [&callable, status = std::move(status)]() { - callable(utils::Unexpected(std::move(status))); + callable(utils::Expected(utils::Unexpected(status))); }); } } @@ -160,7 +178,7 @@ RpcClient::InvokeHandle RpcClient::invokeMethod(v1::UMessage&& request, 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))); + 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( @@ -204,7 +222,7 @@ RpcClient::InvokeFuture RpcClient::invokeMethod( auto promise = std::make_shared>(); auto future = promise->get_future(); auto handle = invokeMethod( - std::move(payload), [promise](MessageOrStatus maybe_message) mutable { + std::move(payload), [promise](const MessageOrStatus& maybe_message) mutable { promise->set_value(maybe_message); }); @@ -219,14 +237,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 +262,19 @@ RpcClient::InvokeFuture::InvokeFuture(std::future&& future, namespace { namespace detail { -using namespace uprotocol; -using namespace std::chrono_literals; +using uprotocol::v1::UStatus; +using uprotocol::v1::UCode; +// 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 +283,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? + // 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) && + constexpr size_t CAPACITY_SHRINK_THRESHOLD = 16; + if ((c.capacity() > CAPACITY_SHRINK_THRESHOLD) && (c.size() < c.capacity() / 2)) { c.shrink_to_fit(); } @@ -304,29 +324,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 +359,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 +378,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 +399,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..9d3b10013 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,12 @@ 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 +143,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..0c3903d79 100644 --- a/src/communication/Subscriber.cpp +++ b/src/communication/Subscriber.cpp @@ -9,12 +9,14 @@ // // SPDX-License-Identifier: Apache-2.0 +#include + #include "up-cpp/communication/Subscriber.h" #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,27 @@ 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..7db0dd720 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 @@ -55,25 +55,21 @@ Payload::Payload(const google::protobuf::Any& 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 +82,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..0e413284a 100644 --- a/src/datamodel/serializer/UUri.cpp +++ b/src/datamodel/serializer/UUri.cpp @@ -18,12 +18,15 @@ namespace uprotocol::datamodel::serializer::uri { std::string AsString::serialize(const v1::UUri& uri) { - using namespace uprotocol::datamodel::validator::uri; + using uprotocol::datamodel::validator::uri::isValidFilter; + using uprotocol::datamodel::validator::uri::InvalidUUri; + using uprotocol::datamodel::validator::uri::isLocal; + // isValidFilter is the most permissive of the validators auto [valid, reason] = isValidFilter(uri); if (!valid) { throw InvalidUUri("Invalid UUri For Serialization | " + - std::string(message(*reason))); + std::string(message(*reason))); } std::stringstream ss; ss << std::hex << std::uppercase; @@ -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,56 +67,57 @@ 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::isValidFilter; + using uprotocol::datamodel::validator::uri::InvalidUUri; // isValidFilter is the most permissive of the validators auto [valid, reason] = isValidFilter(uri); if (!valid) { throw InvalidUUri("Invalid UUri For DeSerialization | " + - std::string(message(*reason))); + std::string(message(*reason))); } } return uri; diff --git a/src/datamodel/serializer/Uuid.cpp b/src/datamodel/serializer/Uuid.cpp index 781a781ee..630c3f04f 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,19 @@ 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 +72,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 +87,45 @@ 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_low = htonl(static_cast(uuid.msb() & MASK_32_BITS)); - uint32_t lsb_high = htonl(static_cast(uuid.lsb() >> 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() >> 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 +135,20 @@ 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..f1503e73a 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::ValidationResult; +using uprotocol::datamodel::validator::uri::Reason; -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,23 @@ 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 +148,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 +185,7 @@ 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 +198,7 @@ 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 +219,7 @@ 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..7a7be6e7f 100644 --- a/src/transport/UTransport.cpp +++ b/src/transport/UTransport.cpp @@ -9,6 +9,8 @@ // // SPDX-License-Identifier: Apache-2.0 +#include + #include "up-cpp/transport/UTransport.h" #include "up-cpp/datamodel/validator/UMessage.h" @@ -17,24 +19,24 @@ 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 +58,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 +89,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 +105,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/client/usubscription/v3/ConsumerTest.cpp b/test/coverage/client/usubscription/v3/ConsumerTest.cpp index 17ba73169..404c85eb6 100644 --- a/test/coverage/client/usubscription/v3/ConsumerTest.cpp +++ b/test/coverage/client/usubscription/v3/ConsumerTest.cpp @@ -82,23 +82,23 @@ class ConsumerTest : public testing::Test { // Negative test case with no source filter TEST_F(ConsumerTest, ConstructorTestSuccess) { - auto subcriptionCallback = someCallBack; + auto subcription_callback = someCallBack; auto subscribe_request_ttl = std::chrono::milliseconds(1000); auto priority = uprotocol::v1::UPriority::UPRIORITY_CS4; auto options = uprotocol::client::usubscription::v3::ConsumerOptions(); - auto consumerOrSatus = + auto consumer_or_status = uprotocol::client::usubscription::v3::Consumer::create( mockTransportClient_, subcription_uuri, - std::move(subcriptionCallback), priority, - std::move(subscribe_request_ttl), options); + subcription_callback, priority, + subscribe_request_ttl, options); // Ensure that the consumer creation was successful - ASSERT_TRUE(consumerOrSatus.has_value()); + ASSERT_TRUE(consumer_or_status.has_value()); // Obtain a pointer to the created consumer instance - auto& consumerPtr = consumerOrSatus.value(); + const auto& consumerPtr = consumer_or_status.value(); // Verify that the consumer pointer is not null, indicating successful // creation @@ -115,8 +115,8 @@ TEST_F(ConsumerTest, SubscribeTestSuccess) { auto consumerOrSatus = uprotocol::client::usubscription::v3::Consumer::create( mockTransportClient_, subcription_uuri, - std::move(subcriptionCallback), priority, - std::move(subscribe_request_ttl), options); + subcriptionCallback, priority, + subscribe_request_ttl, options); // Ensure that the consumer creation was successful ASSERT_TRUE(consumerOrSatus.has_value()); @@ -160,14 +160,14 @@ TEST_F(ConsumerTest, UnsubscribeTestSuccess) { auto consumerOrSatus = uprotocol::client::usubscription::v3::Consumer::create( mockTransportClient_, subcription_uuri, - std::move(subcriptionCallback), priority, - std::move(subscribe_request_ttl), options); + subcriptionCallback, priority, + subscribe_request_ttl, options); // Ensure that the consumer creation was successful ASSERT_TRUE(consumerOrSatus.has_value()); // Obtain a pointer to the created consumer instance - auto& consumerPtr = consumerOrSatus.value(); + const auto& consumerPtr = consumerOrSatus.value(); // Verify that the consumer pointer is not null, indicating successful // creation diff --git a/test/coverage/communication/NotificationSinkTest.cpp b/test/coverage/communication/NotificationSinkTest.cpp index 2a545e36b..0ab196d48 100644 --- a/test/coverage/communication/NotificationSinkTest.cpp +++ b/test/coverage/communication/NotificationSinkTest.cpp @@ -34,7 +34,7 @@ class NotificationSinkTest : public testing::Test { // Run once per execution of the test application. // Used for setup of all tests. Has access to this instance. NotificationSinkTest() = default; - ~NotificationSinkTest() = default; + ~NotificationSinkTest() override = default; void buildDefaultSourceURI(); void buildValidNotificationURI(); diff --git a/test/coverage/communication/NotificationSourceTest.cpp b/test/coverage/communication/NotificationSourceTest.cpp index 1463876ef..0f15622ad 100644 --- a/test/coverage/communication/NotificationSourceTest.cpp +++ b/test/coverage/communication/NotificationSourceTest.cpp @@ -73,17 +73,17 @@ class TestNotificationSource : public testing::Test { }; TEST_F(TestNotificationSource, NotifyWithPayloadSuccess) { - std::string testPayloadStr = "test_payload"; - NotificationSource notificationSource(transportMock_, std::move(source_), + std::string test_payload_str = "test_payload"; + NotificationSource notification_source(transportMock_, std::move(source_), std::move(sink_), format_, priority_, ttl_); - Payload testPayload(testPayloadStr, format_); + Payload test_payload(test_payload_str, format_); uprotocol::v1::UStatus retval; retval.set_code(uprotocol::v1::UCode::OK); transportMock_->send_status_ = retval; - auto status = notificationSource.notify(std::move(testPayload)); + auto status = notification_source.notify(std::move(test_payload)); EXPECT_EQ(status.code(), retval.code()); @@ -94,16 +94,16 @@ TEST_F(TestNotificationSource, NotifyWithPayloadSuccess) { } TEST_F(TestNotificationSource, NotifyWithPayloadSuccessWithoutTTL) { - std::string testPayloadStr = "test_payload"; - NotificationSource notificationSource(transportMock_, std::move(source_), + std::string test_payload_str = "test_payload"; + NotificationSource notification_source(transportMock_, std::move(source_), std::move(sink_), format_, priority_); - Payload testPayload(testPayloadStr, format_); + Payload test_payload(test_payload_str, format_); uprotocol::v1::UStatus retval; retval.set_code(uprotocol::v1::UCode::OK); transportMock_->send_status_ = retval; - auto status = notificationSource.notify(std::move(testPayload)); + auto status = notification_source.notify(std::move(test_payload)); EXPECT_EQ(status.code(), retval.code()); @@ -116,17 +116,17 @@ TEST_F(TestNotificationSource, NotifyWithPayloadSuccessWithoutTTL) { } TEST_F(TestNotificationSource, NotifyWithPayloadSuccessWithoutPriority) { - std::string testPayloadStr = "test_payload"; + std::string test_payload_str = "test_payload"; priority_.reset(); - NotificationSource notificationSource(transportMock_, std::move(source_), + NotificationSource notification_source(transportMock_, std::move(source_), std::move(sink_), format_, priority_); - Payload testPayload(testPayloadStr, format_); + Payload test_payload(test_payload_str, format_); uprotocol::v1::UStatus retval; retval.set_code(uprotocol::v1::UCode::OK); transportMock_->send_status_ = retval; - auto status = notificationSource.notify(std::move(testPayload)); + auto status = notification_source.notify(std::move(test_payload)); EXPECT_EQ(status.code(), retval.code()); @@ -140,30 +140,30 @@ TEST_F(TestNotificationSource, NotifyWithPayloadSuccessWithoutPriority) { } TEST_F(TestNotificationSource, NotifyWithPayloadFailure) { - std::string testPayloadStr = "test_payload"; - NotificationSource notificationSource(transportMock_, std::move(source_), + std::string test_payload_str = "test_payload"; + NotificationSource notification_source(transportMock_, std::move(source_), std::move(sink_), format_, priority_, ttl_); - Payload testPayload(testPayloadStr, format_); + Payload test_payload(test_payload_str, format_); uprotocol::v1::UStatus retval; retval.set_code(uprotocol::v1::UCode::DATA_LOSS); transportMock_->send_status_ = retval; - auto status = notificationSource.notify(std::move(testPayload)); + auto status = notification_source.notify(std::move(test_payload)); EXPECT_EQ(status.code(), retval.code()); } TEST_F(TestNotificationSource, NotifyWithoutPayloadSuccess) { - NotificationSource notificationSource(transportMock_, std::move(source_), + NotificationSource notification_source(transportMock_, std::move(source_), std::move(sink_)); uprotocol::v1::UStatus retval; retval.set_code(uprotocol::v1::UCode::OK); transportMock_->send_status_ = retval; - auto status = notificationSource.notify(); + auto status = notification_source.notify(); EXPECT_EQ(status.code(), retval.code()); @@ -173,14 +173,14 @@ TEST_F(TestNotificationSource, NotifyWithoutPayloadSuccess) { } TEST_F(TestNotificationSource, NotifyWithoutPayloadFailure) { - NotificationSource notificationSource(transportMock_, std::move(source_), + NotificationSource notification_source(transportMock_, std::move(source_), std::move(sink_)); uprotocol::v1::UStatus retval; retval.set_code(uprotocol::v1::UCode::DATA_LOSS); transportMock_->send_status_ = retval; - auto status = notificationSource.notify(); + auto status = notification_source.notify(); EXPECT_EQ(status.code(), retval.code()); } diff --git a/test/coverage/datamodel/UUriValidatorTest.cpp b/test/coverage/datamodel/UUriValidatorTest.cpp index b8acca4e8..d41abc9b7 100644 --- a/test/coverage/datamodel/UUriValidatorTest.cpp +++ b/test/coverage/datamodel/UUriValidatorTest.cpp @@ -489,7 +489,7 @@ TEST_F(TestUUriValidator, ValidDefaultSource) { } TEST_F(TestUUriValidator, Empty) { - auto getUuri = []() { + auto get_uuri = []() { uprotocol::v1::UUri uuri; uuri.set_authority_name(""); uuri.set_ue_id(0); @@ -499,14 +499,14 @@ TEST_F(TestUUriValidator, Empty) { }; { - auto uuri = getUuri(); + auto uuri = get_uuri(); auto [valid, reason] = isEmpty(uuri); EXPECT_TRUE(valid); EXPECT_FALSE(reason.has_value()); } { - auto uuri = getUuri(); + auto uuri = get_uuri(); uuri.set_authority_name(" bad "); auto [valid, reason] = isEmpty(uuri); EXPECT_FALSE(valid); @@ -514,7 +514,7 @@ TEST_F(TestUUriValidator, Empty) { } { - auto uuri = getUuri(); + auto uuri = get_uuri(); uuri.set_authority_name(AUTHORITY_NAME); auto [valid, reason] = isEmpty(uuri); EXPECT_FALSE(valid); @@ -522,7 +522,7 @@ TEST_F(TestUUriValidator, Empty) { } { - auto uuri = getUuri(); + auto uuri = get_uuri(); uuri.set_ue_id(1); auto [valid, reason] = isEmpty(uuri); EXPECT_FALSE(valid); @@ -530,7 +530,7 @@ TEST_F(TestUUriValidator, Empty) { } { - auto uuri = getUuri(); + auto uuri = get_uuri(); uuri.set_ue_version_major(1); auto [valid, reason] = isEmpty(uuri); EXPECT_FALSE(valid); @@ -538,7 +538,7 @@ TEST_F(TestUUriValidator, Empty) { } { - auto uuri = getUuri(); + auto uuri = get_uuri(); uuri.set_resource_id(1); auto [valid, reason] = isEmpty(uuri); EXPECT_FALSE(valid); diff --git a/test/coverage/datamodel/UuidBuilderTest.cpp b/test/coverage/datamodel/UuidBuilderTest.cpp index f6f7d1cea..76162ff63 100644 --- a/test/coverage/datamodel/UuidBuilderTest.cpp +++ b/test/coverage/datamodel/UuidBuilderTest.cpp @@ -64,7 +64,7 @@ TEST(UuidBuilderTest, WithTimeSource) { // Test RandomSource TEST(UuidBuilderTest, WithRandomSource) { - auto fixed_random = 0x1234567890ABCDEF; + uint64_t fixed_random = 0x1234567890ABCDEF; auto builder = UuidBuilder::getTestBuilder().withRandomSource( [fixed_random]() { return fixed_random; }); auto uuid = builder.build(); diff --git a/test/coverage/utils/CallbackConnectionTest.cpp b/test/coverage/utils/CallbackConnectionTest.cpp index fd265b4ac..8751f0570 100644 --- a/test/coverage/utils/CallbackConnectionTest.cpp +++ b/test/coverage/utils/CallbackConnectionTest.cpp @@ -43,7 +43,7 @@ class CallbackTest : public testing::Test { TEST_F(CallbackTest, EstablishDoesNotThrow) { using namespace uprotocol::utils; - EXPECT_NO_THROW(callbacks::Connection::establish([]() {})); + EXPECT_NO_THROW(auto result = callbacks::Connection::establish([]() {})); } /// It should be possible to establish a connection and call the callback diff --git a/test/coverage/utils/ExpectedTest.cpp b/test/coverage/utils/ExpectedTest.cpp index 76f712854..176559919 100644 --- a/test/coverage/utils/ExpectedTest.cpp +++ b/test/coverage/utils/ExpectedTest.cpp @@ -56,7 +56,7 @@ TEST_F(ExpectedTest, ExpectScalarScalar) { TEST_F(ExpectedTest, UnexpectScalarScalar) { int sample = get_rand(); - auto expected = Expected(Unexpected(sample)); + auto expected = Expected(Unexpected(sample)); EXPECT_FALSE(bool(expected)); EXPECT_FALSE(expected.has_value()); EXPECT_EQ(sample, expected.error()); @@ -73,7 +73,7 @@ TEST_F(ExpectedTest, ExpectScalar) { TEST_F(ExpectedTest, UnexpectScalar) { int sample = get_rand(); - auto expected = Expected(Unexpected(sample)); + auto expected = Expected(Unexpected(sample)); EXPECT_FALSE(bool(expected)); EXPECT_FALSE(expected.has_value()); EXPECT_EQ(sample, expected.error()); @@ -82,7 +82,7 @@ TEST_F(ExpectedTest, UnexpectScalar) { TEST_F(ExpectedTest, UnexpectValueOr) { int sample = get_rand(); auto expected = - Expected(Unexpected(std::string("hello"))); + Expected(Unexpected(std::string("hello"))); EXPECT_FALSE(bool(expected)); EXPECT_FALSE(expected.has_value()); EXPECT_EQ(sample, expected.value_or(sample)); @@ -111,7 +111,7 @@ TEST_F(ExpectedTest, UnexpectUnique) { auto x = get_rand(); auto y = get_rand(); auto expected = Expected>( - Unexpected(std::make_unique(x, y))); + Unexpected>(std::make_unique(x, y))); EXPECT_FALSE(bool(expected)); EXPECT_FALSE(expected.has_value()); auto p = std::move(expected).error(); @@ -136,7 +136,7 @@ TEST_F(ExpectedTest, UnexpectShared) { auto x = get_rand(); auto y = get_rand(); auto expected = Expected>( - Unexpected(std::make_shared(x, y))); + Unexpected>(std::make_shared(x, y))); EXPECT_FALSE(bool(expected)); EXPECT_FALSE(expected.has_value()); EXPECT_EQ(x, expected.error()->x); @@ -158,7 +158,7 @@ TEST_F(ExpectedTest, ExpectStruct) { TEST_F(ExpectedTest, UnexpectStruct) { auto x = get_rand(); auto y = get_rand(); - auto expected = Expected(Unexpected(Pair(x, y))); + auto expected = Expected(Unexpected(Pair(x, y))); EXPECT_FALSE(bool(expected)); EXPECT_FALSE(expected.has_value()); EXPECT_EQ(x, expected.error().x); @@ -202,7 +202,7 @@ TEST_F(ExpectedTest, UnexpectStructDestruct) { auto x = get_rand(); auto y = get_rand(); auto expected = - Expected(Unexpected(PairDestruct(x, y))); + Expected(Unexpected(PairDestruct(x, y))); EXPECT_EQ(1, PairDestruct::cd_count); EXPECT_FALSE(bool(expected)); EXPECT_FALSE(expected.has_value()); @@ -214,7 +214,7 @@ TEST_F(ExpectedTest, UnexpectStructDestruct) { TEST_F(ExpectedTest, ExceptionValueCheckedWhenIsError) { auto expected = - Expected(Unexpected(std::string("hello"))); + Expected(Unexpected(std::string("hello"))); EXPECT_THROW( { try { @@ -249,7 +249,7 @@ TEST_F(ExpectedTest, ExceptionErrorCheckedWhenNotError) { TEST_F(ExpectedTest, ExceptionDerefValueWhenUnexpected) { auto expected = - Expected(Unexpected(std::string("hello"))); + Expected(Unexpected(std::string("hello"))); EXPECT_THROW( { try { @@ -268,7 +268,7 @@ TEST_F(ExpectedTest, ExceptionDerefValueWhenUnexpected) { TEST_F(ExpectedTest, ExceptionDerefPtrWhenUnexpected) { auto expected = - Expected(Unexpected(std::string("hello"))); + Expected(Unexpected(std::string("hello"))); EXPECT_THROW( { try { diff --git a/test/include/UTransportMock.h b/test/include/UTransportMock.h index 29129e6c3..ca0b6bff7 100644 --- a/test/include/UTransportMock.h +++ b/test/include/UTransportMock.h @@ -9,8 +9,8 @@ // // SPDX-License-Identifier: Apache-2.0 -#ifndef UP_CPP_TEST_UTRANSPORTMOCK_H -#define UP_CPP_TEST_UTRANSPORTMOCK_H +#ifndef UTRANSPORTMOCK_H +#define UTRANSPORTMOCK_H #include #include @@ -33,6 +33,8 @@ class UTransportMock : public uprotocol::transport::UTransport { (*listener_)(msg); } +//TODO(max) set private again and fix access in RpcServerTest +public: std::atomic send_count_; uprotocol::v1::UStatus send_status_; @@ -51,9 +53,8 @@ class UTransportMock : public uprotocol::transport::UTransport { v1::UMessage message_; std::mutex message_mtx_; - virtual ~UTransportMock() = default; + ~UTransportMock() override = default; -private: [[nodiscard]] v1::UStatus sendImpl(const v1::UMessage& message) override { { std::lock_guard lock(message_mtx_); @@ -73,11 +74,11 @@ class UTransportMock : public uprotocol::transport::UTransport { return registerListener_status_; } - void cleanupListener(CallableConn listener) override { + void cleanupListener(const CallableConn& listener) override { cleanup_listener_ = listener; } }; }; // namespace uprotocol::test -#endif // UP_CPP_TEST_UTRANSPORTMOCK_H +#endif // UTRANSPORTMOCK_H From e4f737bddf3ba6f132b40e57aa9ab952492405ed Mon Sep 17 00:00:00 2001 From: maximiliantoe Date: Mon, 17 Mar 2025 09:16:19 +0100 Subject: [PATCH 2/4] reverted changes in test files since clang-tidy does current not check, need to keep UTransportMock.h --- .../client/usubscription/v3/ConsumerTest.cpp | 22 +++++----- .../communication/NotificationSinkTest.cpp | 2 +- .../communication/NotificationSourceTest.cpp | 40 +++++++++---------- test/coverage/datamodel/UUriValidatorTest.cpp | 14 +++---- test/coverage/datamodel/UuidBuilderTest.cpp | 2 +- .../coverage/utils/CallbackConnectionTest.cpp | 2 +- test/coverage/utils/ExpectedTest.cpp | 20 +++++----- test/include/UTransportMock.h | 11 +++-- 8 files changed, 56 insertions(+), 57 deletions(-) diff --git a/test/coverage/client/usubscription/v3/ConsumerTest.cpp b/test/coverage/client/usubscription/v3/ConsumerTest.cpp index 404c85eb6..17ba73169 100644 --- a/test/coverage/client/usubscription/v3/ConsumerTest.cpp +++ b/test/coverage/client/usubscription/v3/ConsumerTest.cpp @@ -82,23 +82,23 @@ class ConsumerTest : public testing::Test { // Negative test case with no source filter TEST_F(ConsumerTest, ConstructorTestSuccess) { - auto subcription_callback = someCallBack; + auto subcriptionCallback = someCallBack; auto subscribe_request_ttl = std::chrono::milliseconds(1000); auto priority = uprotocol::v1::UPriority::UPRIORITY_CS4; auto options = uprotocol::client::usubscription::v3::ConsumerOptions(); - auto consumer_or_status = + auto consumerOrSatus = uprotocol::client::usubscription::v3::Consumer::create( mockTransportClient_, subcription_uuri, - subcription_callback, priority, - subscribe_request_ttl, options); + std::move(subcriptionCallback), priority, + std::move(subscribe_request_ttl), options); // Ensure that the consumer creation was successful - ASSERT_TRUE(consumer_or_status.has_value()); + ASSERT_TRUE(consumerOrSatus.has_value()); // Obtain a pointer to the created consumer instance - const auto& consumerPtr = consumer_or_status.value(); + auto& consumerPtr = consumerOrSatus.value(); // Verify that the consumer pointer is not null, indicating successful // creation @@ -115,8 +115,8 @@ TEST_F(ConsumerTest, SubscribeTestSuccess) { auto consumerOrSatus = uprotocol::client::usubscription::v3::Consumer::create( mockTransportClient_, subcription_uuri, - subcriptionCallback, priority, - subscribe_request_ttl, options); + std::move(subcriptionCallback), priority, + std::move(subscribe_request_ttl), options); // Ensure that the consumer creation was successful ASSERT_TRUE(consumerOrSatus.has_value()); @@ -160,14 +160,14 @@ TEST_F(ConsumerTest, UnsubscribeTestSuccess) { auto consumerOrSatus = uprotocol::client::usubscription::v3::Consumer::create( mockTransportClient_, subcription_uuri, - subcriptionCallback, priority, - subscribe_request_ttl, options); + std::move(subcriptionCallback), priority, + std::move(subscribe_request_ttl), options); // Ensure that the consumer creation was successful ASSERT_TRUE(consumerOrSatus.has_value()); // Obtain a pointer to the created consumer instance - const auto& consumerPtr = consumerOrSatus.value(); + auto& consumerPtr = consumerOrSatus.value(); // Verify that the consumer pointer is not null, indicating successful // creation diff --git a/test/coverage/communication/NotificationSinkTest.cpp b/test/coverage/communication/NotificationSinkTest.cpp index 0ab196d48..2a545e36b 100644 --- a/test/coverage/communication/NotificationSinkTest.cpp +++ b/test/coverage/communication/NotificationSinkTest.cpp @@ -34,7 +34,7 @@ class NotificationSinkTest : public testing::Test { // Run once per execution of the test application. // Used for setup of all tests. Has access to this instance. NotificationSinkTest() = default; - ~NotificationSinkTest() override = default; + ~NotificationSinkTest() = default; void buildDefaultSourceURI(); void buildValidNotificationURI(); diff --git a/test/coverage/communication/NotificationSourceTest.cpp b/test/coverage/communication/NotificationSourceTest.cpp index 0f15622ad..1463876ef 100644 --- a/test/coverage/communication/NotificationSourceTest.cpp +++ b/test/coverage/communication/NotificationSourceTest.cpp @@ -73,17 +73,17 @@ class TestNotificationSource : public testing::Test { }; TEST_F(TestNotificationSource, NotifyWithPayloadSuccess) { - std::string test_payload_str = "test_payload"; - NotificationSource notification_source(transportMock_, std::move(source_), + std::string testPayloadStr = "test_payload"; + NotificationSource notificationSource(transportMock_, std::move(source_), std::move(sink_), format_, priority_, ttl_); - Payload test_payload(test_payload_str, format_); + Payload testPayload(testPayloadStr, format_); uprotocol::v1::UStatus retval; retval.set_code(uprotocol::v1::UCode::OK); transportMock_->send_status_ = retval; - auto status = notification_source.notify(std::move(test_payload)); + auto status = notificationSource.notify(std::move(testPayload)); EXPECT_EQ(status.code(), retval.code()); @@ -94,16 +94,16 @@ TEST_F(TestNotificationSource, NotifyWithPayloadSuccess) { } TEST_F(TestNotificationSource, NotifyWithPayloadSuccessWithoutTTL) { - std::string test_payload_str = "test_payload"; - NotificationSource notification_source(transportMock_, std::move(source_), + std::string testPayloadStr = "test_payload"; + NotificationSource notificationSource(transportMock_, std::move(source_), std::move(sink_), format_, priority_); - Payload test_payload(test_payload_str, format_); + Payload testPayload(testPayloadStr, format_); uprotocol::v1::UStatus retval; retval.set_code(uprotocol::v1::UCode::OK); transportMock_->send_status_ = retval; - auto status = notification_source.notify(std::move(test_payload)); + auto status = notificationSource.notify(std::move(testPayload)); EXPECT_EQ(status.code(), retval.code()); @@ -116,17 +116,17 @@ TEST_F(TestNotificationSource, NotifyWithPayloadSuccessWithoutTTL) { } TEST_F(TestNotificationSource, NotifyWithPayloadSuccessWithoutPriority) { - std::string test_payload_str = "test_payload"; + std::string testPayloadStr = "test_payload"; priority_.reset(); - NotificationSource notification_source(transportMock_, std::move(source_), + NotificationSource notificationSource(transportMock_, std::move(source_), std::move(sink_), format_, priority_); - Payload test_payload(test_payload_str, format_); + Payload testPayload(testPayloadStr, format_); uprotocol::v1::UStatus retval; retval.set_code(uprotocol::v1::UCode::OK); transportMock_->send_status_ = retval; - auto status = notification_source.notify(std::move(test_payload)); + auto status = notificationSource.notify(std::move(testPayload)); EXPECT_EQ(status.code(), retval.code()); @@ -140,30 +140,30 @@ TEST_F(TestNotificationSource, NotifyWithPayloadSuccessWithoutPriority) { } TEST_F(TestNotificationSource, NotifyWithPayloadFailure) { - std::string test_payload_str = "test_payload"; - NotificationSource notification_source(transportMock_, std::move(source_), + std::string testPayloadStr = "test_payload"; + NotificationSource notificationSource(transportMock_, std::move(source_), std::move(sink_), format_, priority_, ttl_); - Payload test_payload(test_payload_str, format_); + Payload testPayload(testPayloadStr, format_); uprotocol::v1::UStatus retval; retval.set_code(uprotocol::v1::UCode::DATA_LOSS); transportMock_->send_status_ = retval; - auto status = notification_source.notify(std::move(test_payload)); + auto status = notificationSource.notify(std::move(testPayload)); EXPECT_EQ(status.code(), retval.code()); } TEST_F(TestNotificationSource, NotifyWithoutPayloadSuccess) { - NotificationSource notification_source(transportMock_, std::move(source_), + NotificationSource notificationSource(transportMock_, std::move(source_), std::move(sink_)); uprotocol::v1::UStatus retval; retval.set_code(uprotocol::v1::UCode::OK); transportMock_->send_status_ = retval; - auto status = notification_source.notify(); + auto status = notificationSource.notify(); EXPECT_EQ(status.code(), retval.code()); @@ -173,14 +173,14 @@ TEST_F(TestNotificationSource, NotifyWithoutPayloadSuccess) { } TEST_F(TestNotificationSource, NotifyWithoutPayloadFailure) { - NotificationSource notification_source(transportMock_, std::move(source_), + NotificationSource notificationSource(transportMock_, std::move(source_), std::move(sink_)); uprotocol::v1::UStatus retval; retval.set_code(uprotocol::v1::UCode::DATA_LOSS); transportMock_->send_status_ = retval; - auto status = notification_source.notify(); + auto status = notificationSource.notify(); EXPECT_EQ(status.code(), retval.code()); } diff --git a/test/coverage/datamodel/UUriValidatorTest.cpp b/test/coverage/datamodel/UUriValidatorTest.cpp index d41abc9b7..b8acca4e8 100644 --- a/test/coverage/datamodel/UUriValidatorTest.cpp +++ b/test/coverage/datamodel/UUriValidatorTest.cpp @@ -489,7 +489,7 @@ TEST_F(TestUUriValidator, ValidDefaultSource) { } TEST_F(TestUUriValidator, Empty) { - auto get_uuri = []() { + auto getUuri = []() { uprotocol::v1::UUri uuri; uuri.set_authority_name(""); uuri.set_ue_id(0); @@ -499,14 +499,14 @@ TEST_F(TestUUriValidator, Empty) { }; { - auto uuri = get_uuri(); + auto uuri = getUuri(); auto [valid, reason] = isEmpty(uuri); EXPECT_TRUE(valid); EXPECT_FALSE(reason.has_value()); } { - auto uuri = get_uuri(); + auto uuri = getUuri(); uuri.set_authority_name(" bad "); auto [valid, reason] = isEmpty(uuri); EXPECT_FALSE(valid); @@ -514,7 +514,7 @@ TEST_F(TestUUriValidator, Empty) { } { - auto uuri = get_uuri(); + auto uuri = getUuri(); uuri.set_authority_name(AUTHORITY_NAME); auto [valid, reason] = isEmpty(uuri); EXPECT_FALSE(valid); @@ -522,7 +522,7 @@ TEST_F(TestUUriValidator, Empty) { } { - auto uuri = get_uuri(); + auto uuri = getUuri(); uuri.set_ue_id(1); auto [valid, reason] = isEmpty(uuri); EXPECT_FALSE(valid); @@ -530,7 +530,7 @@ TEST_F(TestUUriValidator, Empty) { } { - auto uuri = get_uuri(); + auto uuri = getUuri(); uuri.set_ue_version_major(1); auto [valid, reason] = isEmpty(uuri); EXPECT_FALSE(valid); @@ -538,7 +538,7 @@ TEST_F(TestUUriValidator, Empty) { } { - auto uuri = get_uuri(); + auto uuri = getUuri(); uuri.set_resource_id(1); auto [valid, reason] = isEmpty(uuri); EXPECT_FALSE(valid); diff --git a/test/coverage/datamodel/UuidBuilderTest.cpp b/test/coverage/datamodel/UuidBuilderTest.cpp index 76162ff63..f6f7d1cea 100644 --- a/test/coverage/datamodel/UuidBuilderTest.cpp +++ b/test/coverage/datamodel/UuidBuilderTest.cpp @@ -64,7 +64,7 @@ TEST(UuidBuilderTest, WithTimeSource) { // Test RandomSource TEST(UuidBuilderTest, WithRandomSource) { - uint64_t fixed_random = 0x1234567890ABCDEF; + auto fixed_random = 0x1234567890ABCDEF; auto builder = UuidBuilder::getTestBuilder().withRandomSource( [fixed_random]() { return fixed_random; }); auto uuid = builder.build(); diff --git a/test/coverage/utils/CallbackConnectionTest.cpp b/test/coverage/utils/CallbackConnectionTest.cpp index 8751f0570..fd265b4ac 100644 --- a/test/coverage/utils/CallbackConnectionTest.cpp +++ b/test/coverage/utils/CallbackConnectionTest.cpp @@ -43,7 +43,7 @@ class CallbackTest : public testing::Test { TEST_F(CallbackTest, EstablishDoesNotThrow) { using namespace uprotocol::utils; - EXPECT_NO_THROW(auto result = callbacks::Connection::establish([]() {})); + EXPECT_NO_THROW(callbacks::Connection::establish([]() {})); } /// It should be possible to establish a connection and call the callback diff --git a/test/coverage/utils/ExpectedTest.cpp b/test/coverage/utils/ExpectedTest.cpp index 176559919..76f712854 100644 --- a/test/coverage/utils/ExpectedTest.cpp +++ b/test/coverage/utils/ExpectedTest.cpp @@ -56,7 +56,7 @@ TEST_F(ExpectedTest, ExpectScalarScalar) { TEST_F(ExpectedTest, UnexpectScalarScalar) { int sample = get_rand(); - auto expected = Expected(Unexpected(sample)); + auto expected = Expected(Unexpected(sample)); EXPECT_FALSE(bool(expected)); EXPECT_FALSE(expected.has_value()); EXPECT_EQ(sample, expected.error()); @@ -73,7 +73,7 @@ TEST_F(ExpectedTest, ExpectScalar) { TEST_F(ExpectedTest, UnexpectScalar) { int sample = get_rand(); - auto expected = Expected(Unexpected(sample)); + auto expected = Expected(Unexpected(sample)); EXPECT_FALSE(bool(expected)); EXPECT_FALSE(expected.has_value()); EXPECT_EQ(sample, expected.error()); @@ -82,7 +82,7 @@ TEST_F(ExpectedTest, UnexpectScalar) { TEST_F(ExpectedTest, UnexpectValueOr) { int sample = get_rand(); auto expected = - Expected(Unexpected(std::string("hello"))); + Expected(Unexpected(std::string("hello"))); EXPECT_FALSE(bool(expected)); EXPECT_FALSE(expected.has_value()); EXPECT_EQ(sample, expected.value_or(sample)); @@ -111,7 +111,7 @@ TEST_F(ExpectedTest, UnexpectUnique) { auto x = get_rand(); auto y = get_rand(); auto expected = Expected>( - Unexpected>(std::make_unique(x, y))); + Unexpected(std::make_unique(x, y))); EXPECT_FALSE(bool(expected)); EXPECT_FALSE(expected.has_value()); auto p = std::move(expected).error(); @@ -136,7 +136,7 @@ TEST_F(ExpectedTest, UnexpectShared) { auto x = get_rand(); auto y = get_rand(); auto expected = Expected>( - Unexpected>(std::make_shared(x, y))); + Unexpected(std::make_shared(x, y))); EXPECT_FALSE(bool(expected)); EXPECT_FALSE(expected.has_value()); EXPECT_EQ(x, expected.error()->x); @@ -158,7 +158,7 @@ TEST_F(ExpectedTest, ExpectStruct) { TEST_F(ExpectedTest, UnexpectStruct) { auto x = get_rand(); auto y = get_rand(); - auto expected = Expected(Unexpected(Pair(x, y))); + auto expected = Expected(Unexpected(Pair(x, y))); EXPECT_FALSE(bool(expected)); EXPECT_FALSE(expected.has_value()); EXPECT_EQ(x, expected.error().x); @@ -202,7 +202,7 @@ TEST_F(ExpectedTest, UnexpectStructDestruct) { auto x = get_rand(); auto y = get_rand(); auto expected = - Expected(Unexpected(PairDestruct(x, y))); + Expected(Unexpected(PairDestruct(x, y))); EXPECT_EQ(1, PairDestruct::cd_count); EXPECT_FALSE(bool(expected)); EXPECT_FALSE(expected.has_value()); @@ -214,7 +214,7 @@ TEST_F(ExpectedTest, UnexpectStructDestruct) { TEST_F(ExpectedTest, ExceptionValueCheckedWhenIsError) { auto expected = - Expected(Unexpected(std::string("hello"))); + Expected(Unexpected(std::string("hello"))); EXPECT_THROW( { try { @@ -249,7 +249,7 @@ TEST_F(ExpectedTest, ExceptionErrorCheckedWhenNotError) { TEST_F(ExpectedTest, ExceptionDerefValueWhenUnexpected) { auto expected = - Expected(Unexpected(std::string("hello"))); + Expected(Unexpected(std::string("hello"))); EXPECT_THROW( { try { @@ -268,7 +268,7 @@ TEST_F(ExpectedTest, ExceptionDerefValueWhenUnexpected) { TEST_F(ExpectedTest, ExceptionDerefPtrWhenUnexpected) { auto expected = - Expected(Unexpected(std::string("hello"))); + Expected(Unexpected(std::string("hello"))); EXPECT_THROW( { try { diff --git a/test/include/UTransportMock.h b/test/include/UTransportMock.h index ca0b6bff7..0121dac55 100644 --- a/test/include/UTransportMock.h +++ b/test/include/UTransportMock.h @@ -9,8 +9,8 @@ // // SPDX-License-Identifier: Apache-2.0 -#ifndef UTRANSPORTMOCK_H -#define UTRANSPORTMOCK_H +#ifndef UP_CPP_TEST_UTRANSPORTMOCK_H +#define UP_CPP_TEST_UTRANSPORTMOCK_H #include #include @@ -33,8 +33,6 @@ class UTransportMock : public uprotocol::transport::UTransport { (*listener_)(msg); } -//TODO(max) set private again and fix access in RpcServerTest -public: std::atomic send_count_; uprotocol::v1::UStatus send_status_; @@ -53,8 +51,9 @@ class UTransportMock : public uprotocol::transport::UTransport { v1::UMessage message_; std::mutex message_mtx_; - ~UTransportMock() override = default; + virtual ~UTransportMock() = default; +private: [[nodiscard]] v1::UStatus sendImpl(const v1::UMessage& message) override { { std::lock_guard lock(message_mtx_); @@ -81,4 +80,4 @@ class UTransportMock : public uprotocol::transport::UTransport { }; // namespace uprotocol::test -#endif // UTRANSPORTMOCK_H +#endif // UP_CPP_TEST_UTRANSPORTMOCK_H From c483ed31f367665846f322544801c75ebe2ee24a Mon Sep 17 00:00:00 2001 From: maximiliantoe Date: Tue, 18 Mar 2025 07:34:22 +0100 Subject: [PATCH 3/4] fixed linter version in ci.yml --- .github/workflows/ci.yml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e54857bab..636fe81a0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -431,12 +431,11 @@ jobs: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} with: repo-root: up-cpp - version: 13 ignore: 'test' 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 @@ -451,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 From 1a3b598d34fd7cb0750082c54abf84562e782b9b Mon Sep 17 00:00:00 2001 From: lammjo Date: Thu, 20 Mar 2025 09:03:45 +0100 Subject: [PATCH 4/4] style: fix clang-format errors --- .../up-cpp/client/usubscription/v3/Consumer.h | 25 +++---- include/up-cpp/communication/RpcClient.h | 2 +- include/up-cpp/communication/RpcServer.h | 4 +- include/up-cpp/datamodel/builder/Payload.h | 3 +- .../datamodel/constants/UuidConstants.h | 20 +++--- include/up-cpp/transport/UTransport.h | 2 +- include/up-cpp/utils/CallbackConnection.h | 7 +- include/up-cpp/utils/Expected.h | 23 ++++--- include/up-cpp/utils/ProtoConverter.h | 5 +- lint/clang-format.sh | 8 +-- src/client/usubscription/v3/Consumer.cpp | 14 ++-- src/communication/NotificationSink.cpp | 7 +- src/communication/NotificationSource.cpp | 2 +- src/communication/RpcClient.cpp | 65 ++++++++++--------- src/communication/RpcServer.cpp | 7 +- src/communication/Subscriber.cpp | 7 +- src/datamodel/builder/Payload.cpp | 7 +- src/datamodel/serializer/UUri.cpp | 8 +-- src/datamodel/serializer/Uuid.cpp | 46 ++++++++----- src/datamodel/validator/UUri.cpp | 19 ++++-- src/transport/UTransport.cpp | 9 +-- .../coverage/datamodel/PayloadBuilderTest.cpp | 5 +- 22 files changed, 161 insertions(+), 134 deletions(-) diff --git a/include/up-cpp/client/usubscription/v3/Consumer.h b/include/up-cpp/client/usubscription/v3/Consumer.h index 5a7eb5e2d..2fed6ab4b 100644 --- a/include/up-cpp/client/usubscription/v3/Consumer.h +++ b/include/up-cpp/client/usubscription/v3/Consumer.h @@ -23,10 +23,10 @@ #include namespace uprotocol::client::usubscription::v3 { -using uprotocol::core::usubscription::v3::uSubscription; -using uprotocol::core::usubscription::v3::Update; -using uprotocol::core::usubscription::v3::UnsubscribeRequest; 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 @@ -49,13 +49,12 @@ 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. +/// 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 @@ -124,7 +123,8 @@ 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. @@ -135,7 +135,8 @@ struct Consumer { 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. @@ -158,7 +159,7 @@ struct Consumer { /// @param transport Transport to register with. /// @param subscriber_details Additional details about the subscriber. Consumer(std::shared_ptr transport, - v1::UUri subscription_topic, + v1::UUri subscription_topic, ConsumerOptions consumer_options = {}); private: @@ -207,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); diff --git a/include/up-cpp/communication/RpcClient.h b/include/up-cpp/communication/RpcClient.h index 2b90d6378..c78ab5f94 100644 --- a/include/up-cpp/communication/RpcClient.h +++ b/include/up-cpp/communication/RpcClient.h @@ -163,7 +163,7 @@ struct RpcClient { [[nodiscard]] InvokeFuture invokeMethod(); /// @brief Default move constructor (defined in RpcClient.cpp) - RpcClient(RpcClient&&) noexcept ; + 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 42de21e1d..daeebebec 100644 --- a/include/up-cpp/communication/RpcServer.h +++ b/include/up-cpp/communication/RpcServer.h @@ -91,8 +91,8 @@ struct RpcServer { /// respond() is called. Note that the original request's TTL /// may also still apply. explicit RpcServer(std::shared_ptr transport, - std::optional format = {}, - std::optional ttl = {}); + 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 1f3a91a9a..303b373a5 100644 --- a/include/up-cpp/datamodel/builder/Payload.h +++ b/include/up-cpp/datamodel/builder/Payload.h @@ -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, - v1::UPayloadFormat format); + Payload(const std::vector& value_bytes, v1::UPayloadFormat format); /// @brief Creates a Payload builder with a provided pre-serialized data. /// diff --git a/include/up-cpp/datamodel/constants/UuidConstants.h b/include/up-cpp/datamodel/constants/UuidConstants.h index a291f497e..05eb7d663 100644 --- a/include/up-cpp/datamodel/constants/UuidConstants.h +++ b/include/up-cpp/datamodel/constants/UuidConstants.h @@ -43,7 +43,7 @@ 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 +// 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; @@ -51,18 +51,20 @@ 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 +// 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; +// 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 +// 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; - +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 diff --git a/include/up-cpp/transport/UTransport.h b/include/up-cpp/transport/UTransport.h index d5f6fd17b..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(v1::UUri ); + explicit UTransport(v1::UUri); /// @brief Send a message. /// diff --git a/include/up-cpp/utils/CallbackConnection.h b/include/up-cpp/utils/CallbackConnection.h index b5d58508f..b0dbda58d 100644 --- a/include/up-cpp/utils/CallbackConnection.h +++ b/include/up-cpp/utils/CallbackConnection.h @@ -193,7 +193,8 @@ struct [[nodiscard]] Connection { }; /// @brief Semi-private constructor. Use the static establish() instead. - Connection(std::shared_ptr cb, PrivateConstructToken token [[maybe_unused]]) + Connection(std::shared_ptr cb, + PrivateConstructToken token [[maybe_unused]]) : callback_(cb) {} // Connection is only ever available wrapped in a std::shared_ptr. @@ -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) - [[nodiscard]] 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. diff --git a/include/up-cpp/utils/Expected.h b/include/up-cpp/utils/Expected.h index 153903712..a9a0482ae 100644 --- a/include/up-cpp/utils/Expected.h +++ b/include/up-cpp/utils/Expected.h @@ -41,7 +41,7 @@ template class Unexpected { public: constexpr Unexpected(const Unexpected&) = default; - constexpr Unexpected(Unexpected&&) noexcept= default; + constexpr Unexpected(Unexpected&&) noexcept = default; constexpr explicit Unexpected(const E& rhs) : storage_(rhs) {} constexpr explicit Unexpected(E&& rhs) : storage_(std::move(rhs)) {} @@ -58,9 +58,12 @@ template class Expected { public: 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)) {} + // 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&&) noexcept = default; @@ -82,7 +85,7 @@ class Expected { constexpr const T& value() const& { if (!has_value()) { throw BadExpectedAccess( - "Attempt to access value() when unexpected."); + "Attempt to access value() when unexpected."); } return std::get(storage_); } @@ -90,7 +93,7 @@ class Expected { constexpr T value() && { if (!has_value()) { throw BadExpectedAccess( - "Attempt to access value() when unexpected."); + "Attempt to access value() when unexpected."); } return std::move(std::get(storage_)); } @@ -98,7 +101,7 @@ class Expected { constexpr const E& error() const& { if (has_value()) { throw BadExpectedAccess( - "Attempt to access error() when not unexpected."); + "Attempt to access error() when not unexpected."); } return std::get>(storage_).error(); } @@ -106,7 +109,7 @@ class Expected { constexpr E error() && { if (has_value()) { throw BadExpectedAccess( - "Attempt to access error() when not unexpected."); + "Attempt to access error() when not unexpected."); } return std::move(std::get>(storage_)).error(); } @@ -114,7 +117,7 @@ class Expected { constexpr const T& operator*() const { if (!has_value()) { throw BadExpectedAccess( - "Attempt to dereference expected value when unexpected."); + "Attempt to dereference expected value when unexpected."); } return std::get(storage_); } @@ -122,7 +125,7 @@ class Expected { constexpr const T* operator->() const { if (!has_value()) { throw BadExpectedAccess( - "Attempt to dereference expected pointer when unexpected."); + "Attempt to dereference expected pointer when unexpected."); } return &std::get(storage_); } diff --git a/include/up-cpp/utils/ProtoConverter.h b/include/up-cpp/utils/ProtoConverter.h index 81fd4e13d..ee4123936 100644 --- a/include/up-cpp/utils/ProtoConverter.h +++ b/include/up-cpp/utils/ProtoConverter.h @@ -8,12 +8,11 @@ #include namespace uprotocol::utils { -using uprotocol::core::usubscription::v3::SubscriberInfo; 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 /// @@ -55,5 +54,5 @@ struct ProtoConverter { static UnsubscribeRequest BuildUnSubscribeRequest( const v1::UUri& subscription_topic); }; -}; // namespace uprotocol::utils +}; // namespace uprotocol::utils #endif // UP_CPP_UTILS_PROTOCONVERTER_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/src/client/usubscription/v3/Consumer.cpp b/src/client/usubscription/v3/Consumer.cpp index 4c9775b1e..0217b63ea 100644 --- a/src/client/usubscription/v3/Consumer.cpp +++ b/src/client/usubscription/v3/Consumer.cpp @@ -16,11 +16,12 @@ namespace uprotocol::client::usubscription::v3 { Consumer::Consumer(std::shared_ptr transport, - v1::UUri subscription_topic, + v1::UUri subscription_topic, ConsumerOptions consumer_options) : transport_(std::move(transport)), subscription_topic_(std::move(subscription_topic)), - consumer_options_(std::move(consumer_options)), rpc_client_(nullptr) { + consumer_options_(std::move(consumer_options)), + rpc_client_(nullptr) { // Initialize uSubscriptionUUriBuilder_ uSubscriptionUUriBuilder_ = USubscriptionUUriBuilder(); } @@ -65,8 +66,7 @@ v1::UStatus Consumer::createNotificationSink() { auto notification_topic = uSubscriptionUUriBuilder_.getNotificationUri(); auto result = communication::NotificationSink::create( - transport_, std::move(notification_sink_callback), - notification_topic); + transport_, std::move(notification_sink_callback), notification_topic); if (result.has_value()) { noficationSinkHandle_ = std::move(result).value(); @@ -91,12 +91,12 @@ 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 on_response = [this](const auto& maybe_response) { - if (maybe_response.has_value() && maybe_response.value().has_payload()) { + if (maybe_response.has_value() && + maybe_response.value().has_payload()) { SubscriptionResponse response; if (response.ParseFromString(maybe_response.value().payload())) { if (response.topic().SerializeAsString() == diff --git a/src/communication/NotificationSink.cpp b/src/communication/NotificationSink.cpp index 9d6aa1647..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( - const 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"); @@ -44,8 +44,7 @@ NotificationSink::SinkOrStatus NotificationSink::create( } return SinkOrStatus(std::make_unique( - transport, - std::forward(std::move(listener).value()))); + transport, std::forward(std::move(listener).value()))); } // NOTE: deprecated diff --git a/src/communication/NotificationSource.cpp b/src/communication/NotificationSource.cpp index d9459cf1e..c5cbb863b 100644 --- a/src/communication/NotificationSource.cpp +++ b/src/communication/NotificationSource.cpp @@ -13,7 +13,7 @@ namespace uprotocol::communication { -using uprotocol::datamodel::builder::UMessageBuilder; +using uprotocol::datamodel::builder::UMessageBuilder; NotificationSource::NotificationSource( std::shared_ptr transport, v1::UUri&& source, diff --git a/src/communication/RpcClient.cpp b/src/communication/RpcClient.cpp index ab0c6dbb7..87239a71f 100644 --- a/src/communication/RpcClient.cpp +++ b/src/communication/RpcClient.cpp @@ -28,15 +28,16 @@ struct PendingRequest { 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) {} + 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_; @@ -44,7 +45,6 @@ struct PendingRequest { size_t instance_id_{}; }; - struct ScrubablePendingQueue : public std::priority_queue, std::greater<>> { @@ -82,19 +82,18 @@ struct RpcClient::ExpireService { void enqueue(std::chrono::steady_clock::time_point when_expire, transport::UTransport::ListenHandle&& response_listener, std::function expire) const { - auto pending = detail::PendingRequest(when_expire, - std::move(response_listener), - std::move(expire), - instance_id_); + 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}; - //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 + // 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_; }; @@ -140,8 +139,7 @@ RpcClient::InvokeHandle RpcClient::invokeMethod(v1::UMessage&& request, // attempt to call the callback succeeds. auto callback_once = std::make_shared(); - auto connected_pair = - 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); @@ -163,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::Expected(utils::Unexpected(status))); - }); + std::call_once( + *callback_once, [&callable, status = std::move(status)]() { + callable(utils::Expected( + utils::Unexpected(status))); + }); } } }; @@ -176,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::Expected(utils::Unexpected(reason))); - }); + std::call_once(*callback_once, + [&callable, reason = std::move(reason)]() { + callable(utils::Expected( + utils::Unexpected(reason))); + }); }; /////////////////////////////////////////////////////////////////////////// @@ -221,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](const 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)}; } @@ -262,8 +263,8 @@ RpcClient::InvokeFuture::InvokeFuture(std::future&& future, namespace { namespace detail { -using uprotocol::v1::UStatus; using uprotocol::v1::UCode; +using uprotocol::v1::UStatus; // using namespace std::chrono_literals; using ListenHandle = uprotocol::transport::UTransport::ListenHandle; @@ -303,8 +304,8 @@ auto ScrubablePendingQueue::scrub(size_t instance_id) { }), c.end()); - // TODO(missing_author) - is there a better way to shrink the internal container? - // Maybe instead we should enforce a capacity limit + // 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)) { diff --git a/src/communication/RpcServer.cpp b/src/communication/RpcServer.cpp index 9d3b10013..54c540417 100644 --- a/src/communication/RpcServer.cpp +++ b/src/communication/RpcServer.cpp @@ -125,9 +125,10 @@ 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 - constexpr auto DEFAULT_INSTANCE_ID_WITH_WILDCARD_SERVICE_ID = 0x0000FFFF; - constexpr auto VERSION_MAJOR_WILDCARD = 0xFF; - constexpr auto RESOURCE_ID_WILDCARD = 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); diff --git a/src/communication/Subscriber.cpp b/src/communication/Subscriber.cpp index 0c3903d79..14ca07a3b 100644 --- a/src/communication/Subscriber.cpp +++ b/src/communication/Subscriber.cpp @@ -9,10 +9,10 @@ // // SPDX-License-Identifier: Apache-2.0 -#include - #include "up-cpp/communication/Subscriber.h" +#include + #include "up-cpp/datamodel/validator/UUri.h" namespace uprotocol::communication { @@ -37,7 +37,8 @@ namespace uri_validator = uprotocol::datamodel::validator::uri; auto handle = transport->registerListener(std::move(callback), topic); if (!handle) { - return SubscriberOrStatus(utils::Unexpected(handle.error())); + return SubscriberOrStatus( + utils::Unexpected(handle.error())); } return SubscriberOrStatus(std::make_unique( diff --git a/src/datamodel/builder/Payload.cpp b/src/datamodel/builder/Payload.cpp index 7db0dd720..865ea9ff5 100644 --- a/src/datamodel/builder/Payload.cpp +++ b/src/datamodel/builder/Payload.cpp @@ -49,8 +49,8 @@ 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 @@ -58,8 +58,7 @@ Payload::Payload(Payload&& other) noexcept : payload_(std::move(other.payload_)), moved_(other.moved_) {} // Copy constructor -Payload::Payload(const Payload& other)=default; - +Payload::Payload(const Payload& other) = default; // Move assignment operator Payload& Payload::operator=(Payload&& other) noexcept { diff --git a/src/datamodel/serializer/UUri.cpp b/src/datamodel/serializer/UUri.cpp index 0e413284a..4b0351fe7 100644 --- a/src/datamodel/serializer/UUri.cpp +++ b/src/datamodel/serializer/UUri.cpp @@ -18,15 +18,15 @@ namespace uprotocol::datamodel::serializer::uri { std::string AsString::serialize(const v1::UUri& uri) { - using uprotocol::datamodel::validator::uri::isValidFilter; 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) { throw InvalidUUri("Invalid UUri For Serialization | " + - std::string(message(*reason))); + std::string(message(*reason))); } std::stringstream ss; ss << std::hex << std::uppercase; @@ -111,13 +111,13 @@ uprotocol::v1::UUri AsString::deserialize(const std::string& uri_as_string) { uri.set_resource_id(segmentToUint32(uri_view)); { - using uprotocol::datamodel::validator::uri::isValidFilter; 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) { throw InvalidUUri("Invalid UUri For DeSerialization | " + - std::string(message(*reason))); + std::string(message(*reason))); } } return uri; diff --git a/src/datamodel/serializer/Uuid.cpp b/src/datamodel/serializer/Uuid.cpp index 630c3f04f..26c0f0dbf 100644 --- a/src/datamodel/serializer/Uuid.cpp +++ b/src/datamodel/serializer/Uuid.cpp @@ -41,7 +41,8 @@ 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(LEN_32_BITS_IN_HEX) - << ((unix_ts_ms >> LEN_LSB_IN_HEX*LEN_HEX_TO_BIT) & MASK_32_BITS) // First 32 bits of timestamp + << ((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 @@ -50,7 +51,7 @@ std::string AsString::serialize(const uprotocol::v1::UUID& uuid) { (rand_a & UUID_RANDOM_A_MASK)) // Last 16 bits of timestamp and version << "-" << std::setw(LEN_16_BITS_IN_HEX) - << (((var & UUID_VARIANT_MASK) << (LEN_HEX_TO_BIT*3+2)) | + << (((var & UUID_VARIANT_MASK) << (LEN_HEX_TO_BIT * 3 + 2)) | ((rand_b >> RANDOM_B_SHIFT) & MASK_14_BITS)) // Variant and randb << "-" << std::setw(LEN_48_BITS_IN_HEX) << (rand_b & UUID_TIMESTAMP_MASK); // Random number @@ -72,9 +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() != TOTAL_UUID_LENGTH || str[POS_FIRST_SEPARATOR] != '-' || str[POS_SECOND_SEPARATOR] != '-' || - str[POS_THIRD_SEPARATOR] != '-' || str[POS_FOURTH_SEPARATOR] != '-') { + 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"); } @@ -87,20 +88,27 @@ 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, 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); + 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(POS_SECOND_SEPARATOR +1, LEN_VCANT_IN_HEX), nullptr, HEX_BASE)); - ver = (msb_low >> LEN_HEX_TO_BIT*3) & 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(POS_THIRD_SEPARATOR +1, LEN_VARR_IN_HEX), nullptr, HEX_BASE)); - var = (var_randb >> (LEN_HEX_TO_BIT*3 +2)) & 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(POS_FOURTH_SEPARATOR+1), 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"); } @@ -117,9 +125,11 @@ uprotocol::v1::UUID AsString::deserialize(const std::string& str) { std::vector AsBytes::serialize(const v1::UUID& uuid) { std::vector bytes(UUID_BYTE_SIZE); - 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() >> LEN_UINT64_IN_BIT/2)); + 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() >> 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); @@ -146,9 +156,11 @@ v1::UUID AsBytes::deserialize(const std::vector& bytes) { std::memcpy(&lsb_low, &bytes[LSB_LOW], UUID_PART_SIZE); uint64_t msb = - (static_cast(ntohl(msb_high)) << LEN_UINT64_IN_BIT/2) | ntohl(msb_low); + (static_cast(ntohl(msb_high)) << LEN_UINT64_IN_BIT / 2) | + ntohl(msb_low); uint64_t lsb = - (static_cast(ntohl(lsb_high)) << LEN_UINT64_IN_BIT/2) | 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/UUri.cpp b/src/datamodel/validator/UUri.cpp index f1503e73a..f623b0f66 100644 --- a/src/datamodel/validator/UUri.cpp +++ b/src/datamodel/validator/UUri.cpp @@ -14,12 +14,12 @@ namespace { constexpr size_t AUTHORITY_SPEC_MAX_LENGTH = 128; -//TODO(max) try to find a better name +// 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::ValidationResult; using uprotocol::datamodel::validator::uri::Reason; +using uprotocol::datamodel::validator::uri::ValidationResult; ValidationResult uriCommonValidChecks(const uprotocol::v1::UUri& uuri) { if (uuri.ue_version_major() == 0) { @@ -80,10 +80,12 @@ bool uses_wildcards(const v1::UUri& uuri) { if (uuri.authority_name().find_first_of('*') != std::string::npos) { return true; } - if ((uuri.ue_id() & LOWER_16_BIT_MASK) == LOWER_16_BIT_MASK) { // service ID + if ((uuri.ue_id() & LOWER_16_BIT_MASK) == + LOWER_16_BIT_MASK) { // service ID return true; } - if ((uuri.ue_id() & UPPER_16_BIT_MASK) == UPPER_16_BIT_MASK) { // 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() == LOWER_8_BIT_MASK) { @@ -185,7 +187,8 @@ ValidationResult isValidPublishTopic(const v1::UUri& uuri) { return {false, Reason::DISALLOWED_WILDCARD}; } - if ((uuri.resource_id() < START_OF_TOPICS) || (uuri.resource_id() > MAX_RESOURCE_ID)) { + if ((uuri.resource_id() < START_OF_TOPICS) || + (uuri.resource_id() > MAX_RESOURCE_ID)) { return {false, Reason::BAD_RESOURCE_ID}; } @@ -198,7 +201,8 @@ ValidationResult isValidNotificationSource(const v1::UUri& uuri) { return {false, Reason::DISALLOWED_WILDCARD}; } - if ((uuri.resource_id() < START_OF_TOPICS) || (uuri.resource_id() > MAX_RESOURCE_ID)) { + if ((uuri.resource_id() < START_OF_TOPICS) || + (uuri.resource_id() > MAX_RESOURCE_ID)) { return {false, Reason::BAD_RESOURCE_ID}; } @@ -219,7 +223,8 @@ ValidationResult isValidNotificationSink(const v1::UUri& uuri) { } ValidationResult isValidSubscription(const v1::UUri& uuri) { - if (uuri.resource_id() < START_OF_TOPICS || uuri.resource_id() > MAX_RESOURCE_ID) { + if (uuri.resource_id() < START_OF_TOPICS || + uuri.resource_id() > MAX_RESOURCE_ID) { return {false, Reason::BAD_RESOURCE_ID}; } diff --git a/src/transport/UTransport.cpp b/src/transport/UTransport.cpp index 7a7be6e7f..2f51eb823 100644 --- a/src/transport/UTransport.cpp +++ b/src/transport/UTransport.cpp @@ -9,10 +9,10 @@ // // SPDX-License-Identifier: Apache-2.0 -#include - #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" @@ -22,7 +22,8 @@ namespace uprotocol::transport { namespace uri_validator = uprotocol::datamodel::validator::uri; namespace message_validator = uprotocol::datamodel::validator::message; -UTransport::UTransport(v1::UUri entity_uri) : entity_uri_(std::move(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 uri_validator::InvalidUUri( @@ -110,7 +111,7 @@ UTransport::registerListener(const v1::UUri& sink_or_topic_filter, } v1::UUri sink_filter_copy = sink_or_topic_filter; return registerListener(std::move(listener), *source_filter, - std::move(sink_filter_copy)); + std::move(sink_filter_copy)); } const v1::UUri& UTransport::getEntityUri() const { return entity_uri_; } 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.