From f24b44782ef38c92be293538c9a9561ba5c980f8 Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Wed, 25 Dec 2024 15:00:30 +1100 Subject: [PATCH 1/3] Address some issues found by sonarqube --- src/clock.cpp | 2 +- src/clock.hpp | 15 ++++++++------- src/dsl/operation/TypeBind.hpp | 4 ++-- src/dsl/word/Always.hpp | 2 +- src/dsl/word/Every.hpp | 2 +- src/dsl/word/IO.hpp | 2 +- src/dsl/word/Idle.hpp | 2 +- src/dsl/word/Network.hpp | 2 +- src/dsl/word/TCP.hpp | 2 +- src/dsl/word/UDP.hpp | 4 ++-- src/dsl/word/Watchdog.hpp | 4 ++-- src/extension/ChronoController.cpp | 2 +- src/extension/NetworkController.cpp | 10 +++++----- src/extension/network/NUClearNetwork.cpp | 4 ++-- src/threading/Reaction.cpp | 2 +- src/util/Logger.hpp | 2 +- src/util/get_hostname.cpp | 4 ++-- src/util/serialise/xxhash.cpp | 4 +--- src/util/serialise/xxhash.hpp | 22 ++++++++++++++++------ src/util/string_join.hpp | 4 +++- 20 files changed, 53 insertions(+), 42 deletions(-) diff --git a/src/clock.cpp b/src/clock.cpp index e5990b8f..3e5831b1 100644 --- a/src/clock.cpp +++ b/src/clock.cpp @@ -45,7 +45,7 @@ void clock::adjust_clock(const duration& adjustment, const double& rtf) { // Load the current state const int c = active.load(std::memory_order_relaxed); const auto& current = data[c]; - const int n = int((c + 1) % data.size()); + const auto n = int((c + 1) % data.size()); auto& next = data[n]; // Perform the update diff --git a/src/clock.hpp b/src/clock.hpp index 6cdcdc4f..a92f7fc6 100644 --- a/src/clock.hpp +++ b/src/clock.hpp @@ -23,11 +23,6 @@ #ifndef NUCLEAR_CLOCK_HPP #define NUCLEAR_CLOCK_HPP -// Default to using the system clock but allow it to be overridden by the user -#ifndef NUCLEAR_CLOCK_TYPE - #define NUCLEAR_CLOCK_TYPE std::chrono::system_clock -#endif // NUCLEAR_CLOCK_TYPE - #include #include #include @@ -35,11 +30,17 @@ namespace NUClear { +// Default to using the system clock but allow it to be overridden by the user +#ifdef NUCLEAR_CLOCK_TYPE +using base_clock = NUCLEAR_CLOCK_TYPE; +#else +using base_clock = std::chrono::steady_clock; +#endif // NUCLEAR_CLOCK_TYPE + /** * A clock class that extends a base clock type and allows for clock adjustment and setting. */ -struct clock : NUCLEAR_CLOCK_TYPE { - using base_clock = NUCLEAR_CLOCK_TYPE; +struct clock : base_clock { /** * Get the current time of the clock. diff --git a/src/dsl/operation/TypeBind.hpp b/src/dsl/operation/TypeBind.hpp index 04f49763..fa76696f 100644 --- a/src/dsl/operation/TypeBind.hpp +++ b/src/dsl/operation/TypeBind.hpp @@ -61,10 +61,10 @@ namespace dsl { static void bind(const std::shared_ptr& reaction) { // Set this reaction as no stats emitting - reaction->emit_stats &= EmitStats::value; + reaction->emit_stats = reaction->emit_stats && EmitStats::value; // Our unbinder to remove this reaction - reaction->unbinders.push_back([](const threading::Reaction& r) { + reaction->unbinders.emplace_back([](const threading::Reaction& r) { auto& vec = store::TypeCallbackStore::get(); auto it = std::find_if( diff --git a/src/dsl/word/Always.hpp b/src/dsl/word/Always.hpp index 9292f739..b522ae0e 100644 --- a/src/dsl/word/Always.hpp +++ b/src/dsl/word/Always.hpp @@ -99,7 +99,7 @@ namespace dsl { static void bind(const std::shared_ptr& reaction) { // Create an unbinder for the always reaction - reaction->unbinders.push_back([](threading::Reaction& r) { + reaction->unbinders.emplace_back([](threading::Reaction& r) { r.enabled = false; // TODO(Alex/Trent) Clean up thread pool too }); diff --git a/src/dsl/word/Every.hpp b/src/dsl/word/Every.hpp index 2802b9d3..4227f231 100644 --- a/src/dsl/word/Every.hpp +++ b/src/dsl/word/Every.hpp @@ -84,7 +84,7 @@ namespace dsl { template static void bind(const std::shared_ptr& reaction, NUClear::clock::duration jump) { - reaction->unbinders.push_back([](const threading::Reaction& r) { + reaction->unbinders.emplace_back([](const threading::Reaction& r) { r.reactor.emit(std::make_unique>(r.id)); }); diff --git a/src/dsl/word/IO.hpp b/src/dsl/word/IO.hpp index 6b7f3cb0..a3644988 100644 --- a/src/dsl/word/IO.hpp +++ b/src/dsl/word/IO.hpp @@ -131,7 +131,7 @@ namespace dsl { template static void bind(const std::shared_ptr& reaction, fd_t fd, event_t watch_set) { - reaction->unbinders.push_back([](const threading::Reaction& r) { + reaction->unbinders.emplace_back([](const threading::Reaction& r) { r.reactor.emit(std::make_unique>(r.id)); }); diff --git a/src/dsl/word/Idle.hpp b/src/dsl/word/Idle.hpp index 7b3763f3..fb95620b 100644 --- a/src/dsl/word/Idle.hpp +++ b/src/dsl/word/Idle.hpp @@ -46,7 +46,7 @@ namespace dsl { const std::shared_ptr& pool_descriptor) { // Our unbinder to remove this reaction - reaction->unbinders.push_back([pool_descriptor](const threading::Reaction& r) { // + reaction->unbinders.emplace_back([pool_descriptor](const threading::Reaction& r) { // r.reactor.powerplant.remove_idle_task(r.id, pool_descriptor); }); diff --git a/src/dsl/word/Network.hpp b/src/dsl/word/Network.hpp index 0f371fc8..113d5fda 100644 --- a/src/dsl/word/Network.hpp +++ b/src/dsl/word/Network.hpp @@ -79,7 +79,7 @@ namespace dsl { auto task = std::make_unique(); task->hash = util::serialise::Serialise::hash(); - reaction->unbinders.push_back([](const threading::Reaction& r) { + reaction->unbinders.emplace_back([](const threading::Reaction& r) { r.reactor.emit(std::make_unique>(r.id)); }); diff --git a/src/dsl/word/TCP.hpp b/src/dsl/word/TCP.hpp index 790a9ddf..8751c996 100644 --- a/src/dsl/word/TCP.hpp +++ b/src/dsl/word/TCP.hpp @@ -147,7 +147,7 @@ namespace dsl { // Generate a reaction for the IO system that closes on death const fd_t cfd = fd.release(); - reaction->unbinders.push_back([cfd](const threading::Reaction&) { + reaction->unbinders.emplace_back([cfd](const threading::Reaction&) { ::shutdown(cfd, SHUT_RDWR); ::close(cfd); }); diff --git a/src/dsl/word/UDP.hpp b/src/dsl/word/UDP.hpp index 0bff35fe..f0762102 100644 --- a/src/dsl/word/UDP.hpp +++ b/src/dsl/word/UDP.hpp @@ -329,7 +329,7 @@ namespace dsl { // Generate a reaction for the IO system that closes on death const fd_t cfd = fd.release(); - reaction->unbinders.push_back([cfd](const threading::Reaction&) { ::close(cfd); }); + reaction->unbinders.emplace_back([cfd](const threading::Reaction&) { ::close(cfd); }); IO::bind(reaction, cfd, IO::READ | IO::CLOSE); // Return our handles and our bound port @@ -520,7 +520,7 @@ namespace dsl { const auto& a = result.local; const bool multicast = - (a.sock.sa_family == AF_INET && (ntohl(a.ipv4.sin_addr.s_addr) & 0xF0000000) == 0xE0000000) + (a.sock.sa_family == AF_INET && (ntohl(a.ipv4.sin_addr.s_addr) & 0xF0000000U) == 0xE0000000U) || (a.sock.sa_family == AF_INET6 && a.ipv6.sin6_addr.s6_addr[0] == 0xFF); // Only return multicast packets diff --git a/src/dsl/word/Watchdog.hpp b/src/dsl/word/Watchdog.hpp index bdff51fa..9d2fab96 100644 --- a/src/dsl/word/Watchdog.hpp +++ b/src/dsl/word/Watchdog.hpp @@ -210,7 +210,7 @@ namespace dsl { WatchdogDataStore::init(data); // Create our unbinder - reaction->unbinders.push_back([data](const threading::Reaction& r) { + reaction->unbinders.emplace_back([data](const threading::Reaction& r) { // Remove the active service time from the data store WatchdogDataStore::unbind(data); r.reactor.emit(std::make_unique>(r.id)); @@ -237,7 +237,7 @@ namespace dsl { WatchdogDataStore::init(); // Create our unbinder - reaction->unbinders.push_back([](const threading::Reaction& r) { + reaction->unbinders.emplace_back([](const threading::Reaction& r) { // Remove the active service time from the data store WatchdogDataStore::unbind(); r.reactor.emit(std::make_unique>(r.id)); diff --git a/src/extension/ChronoController.cpp b/src/extension/ChronoController.cpp index f7c01bf3..2866e25d 100644 --- a/src/extension/ChronoController.cpp +++ b/src/extension/ChronoController.cpp @@ -158,7 +158,7 @@ namespace extension { if (clock::rtf() == 0.0) { // If we are paused then just wait until we are unpaused - wait.wait(lock, [&] { + wait.wait(lock, [this, &start] { return !running.load(std::memory_order_acquire) || clock::rtf() != 0.0 || NUClear::clock::now() != start; }); diff --git a/src/extension/NetworkController.cpp b/src/extension/NetworkController.cpp index 1af37262..429cd9bc 100644 --- a/src/extension/NetworkController.cpp +++ b/src/extension/NetworkController.cpp @@ -107,11 +107,11 @@ namespace extension { const std::lock_guard lock(reaction_mutex); // Find and delete this reaction - for (auto it = reactions.begin(); it != reactions.end(); ++it) { - if (it->second->id == unbind.id) { - reactions.erase(it); - break; - } + auto it = std::find_if(reactions.begin(), reactions.end(), [&](const auto& r) { + return r.second->id == unbind.id; + }); + if (it != reactions.end()) { + reactions.erase(it); } }); diff --git a/src/extension/network/NUClearNetwork.cpp b/src/extension/network/NUClearNetwork.cpp index 778adaa1..b7f21d79 100644 --- a/src/extension/network/NUClearNetwork.cpp +++ b/src/extension/network/NUClearNetwork.cpp @@ -189,7 +189,7 @@ namespace extension { // Work out what type of announce we are doing as it will influence how we make the socket const bool multicast = (announce_target.sock.sa_family == AF_INET - && (ntohl(announce_target.ipv4.sin_addr.s_addr) & 0xF0000000) == 0xE0000000) + && (ntohl(announce_target.ipv4.sin_addr.s_addr) & 0xF0000000U) == 0xE0000000) || (announce_target.sock.sa_family == AF_INET6 && announce_target.ipv6.sin6_addr.s6_addr[0] == 0xFF); // Make our socket @@ -920,7 +920,7 @@ namespace extension { ? 0xFF : 0xFF >> (8 - (packet.packet_count % 8)); - all_acked &= static_cast((s->acked[i] & expected) == expected); + all_acked = all_acked && static_cast((s->acked[i] & expected) == expected); } // The remote has received this entire packet we can erase our sender diff --git a/src/threading/Reaction.cpp b/src/threading/Reaction.cpp index a3b10ba8..31892dd0 100644 --- a/src/threading/Reaction.cpp +++ b/src/threading/Reaction.cpp @@ -51,7 +51,7 @@ namespace threading { void Reaction::unbind() { // Unbind - for (auto& u : unbinders) { + for (const auto& u : unbinders) { u(*this); } } diff --git a/src/util/Logger.hpp b/src/util/Logger.hpp index d9bb3271..9fae6377 100644 --- a/src/util/Logger.hpp +++ b/src/util/Logger.hpp @@ -58,7 +58,7 @@ namespace util { auto log_levels = get_current_log_levels(reactor); if (level >= log_levels.display_log_level || level >= log_levels.min_log_level) { // Collapse the arguments into a string and perform the log action - do_log(reactor, level, string_join(" ", std::forward(args)...)); + do_log(reactor, level, string_join(" ", args...)); } } diff --git a/src/util/get_hostname.cpp b/src/util/get_hostname.cpp index 2cd47c5d..0a32889f 100644 --- a/src/util/get_hostname.cpp +++ b/src/util/get_hostname.cpp @@ -40,8 +40,8 @@ std::string NUClear::util::get_hostname() { #include std::string NUClear::util::get_hostname() { - utsname u{}; - uname(&u); + ::utsname u{}; + ::uname(&u); return u.nodename; } diff --git a/src/util/serialise/xxhash.cpp b/src/util/serialise/xxhash.cpp index d8e86de6..1211ba36 100644 --- a/src/util/serialise/xxhash.cpp +++ b/src/util/serialise/xxhash.cpp @@ -100,7 +100,7 @@ namespace util { } // namespace - uint32_t xxhash32(const void* input_v, const size_t& length, const uint32_t& seed) { + uint32_t xxhash32(const char* input, const size_t& length, const uint32_t& seed) { static constexpr uint32_t PRIME1 = 0x9E3779B1U; static constexpr uint32_t PRIME2 = 0x85EBCA77U; @@ -108,8 +108,6 @@ namespace util { static constexpr uint32_t PRIME4 = 0x27D4EB2FU; static constexpr uint32_t PRIME5 = 0x165667B1U; - // Cast the input pointer to a character pointer. - const auto* input = static_cast(input_v); /// The hash value being calculated. uint32_t h{}; /// A pointer to the current position in the input buffer. diff --git a/src/util/serialise/xxhash.hpp b/src/util/serialise/xxhash.hpp index fc065c6a..515c0abc 100644 --- a/src/util/serialise/xxhash.hpp +++ b/src/util/serialise/xxhash.hpp @@ -31,9 +31,9 @@ namespace util { namespace serialise { /** - * Calculates the 32-bit xxHash of the input data. + * Calculates the 32-bit xxhash of the input data. * - * This function calculates the 32-bit xxHash of the input data using the specified seed value. + * This function calculates the 32-bit xxhash of the input data using the specified seed value. * * @param input Pointer to the input data. * @param length Length of the input data in bytes. @@ -41,12 +41,17 @@ namespace util { * * @return The 32-bit xxHash of the input data. */ - uint32_t xxhash32(const void* input, const size_t& length, const uint32_t& seed = 0); + uint32_t xxhash32(const char* input, const size_t& length, const uint32_t& seed = 0); + + template + uint32_t xxhash32(const T* input, const uint32_t& seed = 0) { + return xxhash32(reinterpret_cast(input), sizeof(T), seed); + } /** - * Calculates the 64-bit xxHash of the input data. + * Calculates the 64-bit xxhash of the input data. * - * This function calculates the 64-bit xxHash of the input data using the specified seed value. + * This function calculates the 64-bit xxhash of the input data using the specified seed value. * * @param input Pointer to the input data. * @param length Length of the input data in bytes. @@ -54,7 +59,12 @@ namespace util { * * @return The 64-bit xxHash of the input data. */ - uint64_t xxhash64(const void* input, const size_t& length, const uint64_t& seed = 0); + uint64_t xxhash64(const char* input, const size_t& length, const uint64_t& seed = 0); + + template + uint64_t xxhash64(const T* input, const uint64_t& seed = 0) { + return xxhash64(reinterpret_cast(input), sizeof(T), seed); + } } // namespace serialise } // namespace util diff --git a/src/util/string_join.hpp b/src/util/string_join.hpp index d5b230b2..36a44502 100644 --- a/src/util/string_join.hpp +++ b/src/util/string_join.hpp @@ -31,7 +31,9 @@ namespace util { namespace detail { // No argument base case - inline void do_string_join(std::stringstream& /*out*/, const std::string& /*delimiter*/) {} + inline void do_string_join(std::stringstream& /*out*/, const std::string& /*delimiter*/) { + // Terminating case with no arguments, do nothing + } // Single argument case template From 7da03829813ddeb3a366116b25a1a80cc5e49d9b Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Wed, 25 Dec 2024 15:19:42 +1100 Subject: [PATCH 2/3] . --- src/util/serialise/xxhash.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/util/serialise/xxhash.cpp b/src/util/serialise/xxhash.cpp index 1211ba36..c1b38fd5 100644 --- a/src/util/serialise/xxhash.cpp +++ b/src/util/serialise/xxhash.cpp @@ -154,7 +154,7 @@ namespace util { return h; } - uint64_t xxhash64(const void* input_v, const size_t& length, const uint64_t& seed) { + uint64_t xxhash64(const char* input, const size_t& length, const uint64_t& seed) { static constexpr uint64_t PRIME1 = 11400714785074694791ULL; static constexpr uint64_t PRIME2 = 14029467366897019727ULL; @@ -162,8 +162,6 @@ namespace util { static constexpr uint64_t PRIME4 = 9650029242287828579ULL; static constexpr uint64_t PRIME5 = 2870177450012600261ULL; - // Cast the input pointer to a character pointer. - const auto* input = static_cast(input_v); /// The hash value being calculated. uint64_t h{}; /// A pointer to the current position in the input buffer. From 6e1d21a0152ae2b45c859598d8eb082b05483c22 Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Wed, 25 Dec 2024 15:25:45 +1100 Subject: [PATCH 3/3] . --- src/extension/network/NUClearNetwork.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/extension/network/NUClearNetwork.cpp b/src/extension/network/NUClearNetwork.cpp index b7f21d79..74ba0e82 100644 --- a/src/extension/network/NUClearNetwork.cpp +++ b/src/extension/network/NUClearNetwork.cpp @@ -920,7 +920,7 @@ namespace extension { ? 0xFF : 0xFF >> (8 - (packet.packet_count % 8)); - all_acked = all_acked && static_cast((s->acked[i] & expected) == expected); + all_acked = all_acked && ((s->acked[i] & expected) == expected); } // The remote has received this entire packet we can erase our sender