From b959901a21acdd1daab8728d22eb0ad3dfc6857c Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Mon, 12 Aug 2024 16:00:18 +1000 Subject: [PATCH] Bunch of clang-tidy stuff --- src/LogLevel.cpp | 55 ++++++++++++++++++++++++ src/LogLevel.hpp | 32 +++++++++++++- src/dsl/word/IO.hpp | 4 +- src/dsl/word/Last.hpp | 2 + src/dsl/word/TCP.hpp | 1 - src/dsl/word/UDP.hpp | 3 +- src/dsl/word/emit/Direct.hpp | 1 - src/dsl/word/emit/Local.hpp | 1 - src/dsl/word/emit/UDP.hpp | 1 - src/dsl/word/emit/Watchdog.hpp | 1 - src/extension/ChronoController.hpp | 1 - src/extension/IOController_Posix.hpp | 1 - src/extension/IOController_Windows.hpp | 1 - src/extension/NetworkController.hpp | 1 - src/extension/network/NUClearNetwork.cpp | 7 +-- src/message/TimeTravel.hpp | 2 +- src/util/FunctionFusion.hpp | 26 ++++++----- src/util/network/get_interfaces.cpp | 4 +- tests/networktest.cpp | 1 + tests/tests/dsl/Priority.cpp | 1 + tests/tests/dsl/TCP.cpp | 4 +- tests/tests/dsl/UDP.cpp | 7 +-- tests/tests/log/Log.cpp | 10 ++--- 23 files changed, 126 insertions(+), 41 deletions(-) create mode 100644 src/LogLevel.cpp diff --git a/src/LogLevel.cpp b/src/LogLevel.cpp new file mode 100644 index 000000000..ee4c276ce --- /dev/null +++ b/src/LogLevel.cpp @@ -0,0 +1,55 @@ +/* + * MIT License + * + * Copyright (c) 2013 NUClear Contributors + * + * This file is part of the NUClear codebase. + * See https://github.com/Fastcode/NUClear for further info. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated + * documentation files (the "Software"), to deal in the Software without restriction, including without limitation the + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to + * permit persons to whom the Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE + * WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +#include "LogLevel.hpp" + +#include + +namespace NUClear { + +std::string to_string(const LogLevel& level) { + switch (level) { + case LogLevel::TRACE: return "TRACE"; + case LogLevel::DEBUG: return "DEBUG"; + case LogLevel::INFO: return "INFO"; + case LogLevel::WARN: return "WARN"; + case LogLevel::ERROR: return "ERROR"; + case LogLevel::FATAL: return "FATAL"; + default: + case LogLevel::UNKNOWN: return "UNKNOWN"; + } +} + +LogLevel from_string(const std::string& level) { + return level == "TRACE" ? LogLevel::TRACE + : level == "DEBUG" ? LogLevel::DEBUG + : level == "INFO" ? LogLevel::INFO + : level == "WARN" ? LogLevel::WARN + : level == "ERROR" ? LogLevel::ERROR + : level == "FATAL" ? LogLevel::FATAL + : LogLevel::UNKNOWN; +} + +std::ostream& operator<<(std::ostream& os, const LogLevel& level) { + return os << to_string(level); +} + +} // namespace NUClear diff --git a/src/LogLevel.hpp b/src/LogLevel.hpp index da7826e26..e0b3c6468 100644 --- a/src/LogLevel.hpp +++ b/src/LogLevel.hpp @@ -19,10 +19,11 @@ * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */ - #ifndef NUCLEAR_LOGLEVEL_HPP #define NUCLEAR_LOGLEVEL_HPP +#include + // Why do we need to include platform.hpp here? // Because windows defines a bunch of things for legacy reasons, one of which is a #define for ERROR as blank // Of course this causes a problem when we define our own token below as error as the preprocessor removes it @@ -37,7 +38,7 @@ namespace NUClear { * Log levels are used to provide different levels of detail on a per-reactor basis. * The logging level of a reactor can be changed by setting it in the install function. */ -enum LogLevel { +enum LogLevel : uint8_t { /** * Don't use this log level when emitting logs, it is for setting reactor log level from non reactor sources. * @@ -97,6 +98,33 @@ enum LogLevel { FATAL }; +/** + * This function is used to convert a LogLevel into a string + * + * @param level the LogLevel to convert + * + * @return the string representation of the LogLevel + */ +std::string to_string(const LogLevel& level); + +/** + * This function is used to convert a string into a LogLevel + * + * @param level the string to convert + * + * @return the LogLevel representation of the string + */ +LogLevel from_string(const std::string& level); + +/** + * This function is used to convert a LogLevel into a string for printing. + * + * @param os the output stream to write to + * @param level the LogLevel to convert + * @return the output stream + */ +std::ostream& operator<<(std::ostream& os, const LogLevel& level); + } // namespace NUClear #endif // NUCLEAR_LOGLEVEL_HPP diff --git a/src/dsl/word/IO.hpp b/src/dsl/word/IO.hpp index c9b2c322f..662d859c1 100644 --- a/src/dsl/word/IO.hpp +++ b/src/dsl/word/IO.hpp @@ -97,7 +97,7 @@ namespace dsl { // On windows we use different wait events #ifdef _WIN32 - // NOLINTNEXTLINE(google-runtime-int) + // NOLINTNEXTLINE(performance-enum-size) these have to be fixed types based on the api enum EventType : event_t { READ = FD_READ | FD_OOB | FD_ACCEPT, WRITE = FD_WRITE, @@ -105,7 +105,7 @@ namespace dsl { ERROR = 0, }; #else - // NOLINTNEXTLINE(google-runtime-int) + // NOLINTNEXTLINE(performance-enum-size) these have to be fixed types based on the api enum EventType : event_t { READ = POLLIN, WRITE = POLLOUT, diff --git a/src/dsl/word/Last.hpp b/src/dsl/word/Last.hpp index c5e0a0220..8ec9bd773 100644 --- a/src/dsl/word/Last.hpp +++ b/src/dsl/word/Last.hpp @@ -26,6 +26,7 @@ #include #include +#include "../../dsl/trait/is_transient.hpp" #include "../../threading/Reaction.hpp" #include "../../util/MergeTransient.hpp" @@ -156,6 +157,7 @@ namespace dsl { private: template + // NOLINTNEXTLINE(cppcoreguidelines-rvalue-reference-param-not-moved) the elements in it are moved static auto wrap(std::tuple&& data, util::Sequence /*s*/) -> decltype(std::make_tuple(LastItemStorage(std::move(std::get(data)))...)) { return std::make_tuple(LastItemStorage(std::move(std::get(data)))...); diff --git a/src/dsl/word/TCP.hpp b/src/dsl/word/TCP.hpp index dd17602c0..8c0a497d0 100644 --- a/src/dsl/word/TCP.hpp +++ b/src/dsl/word/TCP.hpp @@ -25,7 +25,6 @@ #include -#include "../../PowerPlant.hpp" #include "../../threading/Reaction.hpp" #include "../../util/FileDescriptor.hpp" #include "../../util/network/resolve.hpp" diff --git a/src/dsl/word/UDP.hpp b/src/dsl/word/UDP.hpp index 8ade29d80..b7ca39f0d 100644 --- a/src/dsl/word/UDP.hpp +++ b/src/dsl/word/UDP.hpp @@ -26,7 +26,6 @@ #include #include -#include "../../PowerPlant.hpp" #include "../../threading/Reaction.hpp" #include "../../util/FileDescriptor.hpp" #include "../../util/network/get_interfaces.hpp" @@ -71,7 +70,7 @@ namespace dsl { */ struct ConnectOptions { /// The type of connection we are making - enum class Type { UNICAST, BROADCAST, MULTICAST }; + enum class Type : uint8_t { UNICAST, BROADCAST, MULTICAST }; /// The type of connection we are making Type type{}; /// The address we are binding to or empty for any diff --git a/src/dsl/word/emit/Direct.hpp b/src/dsl/word/emit/Direct.hpp index 84cbe4a29..fc71d1faa 100644 --- a/src/dsl/word/emit/Direct.hpp +++ b/src/dsl/word/emit/Direct.hpp @@ -23,7 +23,6 @@ #ifndef NUCLEAR_DSL_WORD_EMIT_DIRECT_HPP #define NUCLEAR_DSL_WORD_EMIT_DIRECT_HPP -#include "../../../PowerPlant.hpp" #include "../../store/DataStore.hpp" #include "../../store/ThreadStore.hpp" #include "../../store/TypeCallbackStore.hpp" diff --git a/src/dsl/word/emit/Local.hpp b/src/dsl/word/emit/Local.hpp index aa93d50d9..20324a43e 100644 --- a/src/dsl/word/emit/Local.hpp +++ b/src/dsl/word/emit/Local.hpp @@ -23,7 +23,6 @@ #ifndef NUCLEAR_DSL_WORD_EMIT_LOCAL_HPP #define NUCLEAR_DSL_WORD_EMIT_LOCAL_HPP -#include "../../../PowerPlant.hpp" #include "../../../util/TypeMap.hpp" #include "../../store/DataStore.hpp" #include "../../store/ThreadStore.hpp" diff --git a/src/dsl/word/emit/UDP.hpp b/src/dsl/word/emit/UDP.hpp index 9e070d2e6..51ea0be90 100644 --- a/src/dsl/word/emit/UDP.hpp +++ b/src/dsl/word/emit/UDP.hpp @@ -25,7 +25,6 @@ #include -#include "../../../PowerPlant.hpp" #include "../../../util/FileDescriptor.hpp" #include "../../../util/network/if_number_from_address.hpp" #include "../../../util/platform.hpp" diff --git a/src/dsl/word/emit/Watchdog.hpp b/src/dsl/word/emit/Watchdog.hpp index 726c04d60..066801157 100644 --- a/src/dsl/word/emit/Watchdog.hpp +++ b/src/dsl/word/emit/Watchdog.hpp @@ -25,7 +25,6 @@ #include -#include "../../../PowerPlant.hpp" #include "../../../util/TypeMap.hpp" #include "../../../util/demangle.hpp" diff --git a/src/extension/ChronoController.hpp b/src/extension/ChronoController.hpp index 7f643e8b8..b5a73f206 100644 --- a/src/extension/ChronoController.hpp +++ b/src/extension/ChronoController.hpp @@ -23,7 +23,6 @@ #ifndef NUCLEAR_EXTENSION_CHRONOCONTROLLER #define NUCLEAR_EXTENSION_CHRONOCONTROLLER -#include "../PowerPlant.hpp" #include "../Reactor.hpp" #include "../message/TimeTravel.hpp" #include "../util/precise_sleep.hpp" diff --git a/src/extension/IOController_Posix.hpp b/src/extension/IOController_Posix.hpp index 8fc6a5597..0be49e66e 100644 --- a/src/extension/IOController_Posix.hpp +++ b/src/extension/IOController_Posix.hpp @@ -30,7 +30,6 @@ #include #include -#include "../PowerPlant.hpp" #include "../Reactor.hpp" #include "../dsl/word/IO.hpp" diff --git a/src/extension/IOController_Windows.hpp b/src/extension/IOController_Windows.hpp index ac1196158..2aaf8344a 100644 --- a/src/extension/IOController_Windows.hpp +++ b/src/extension/IOController_Windows.hpp @@ -23,7 +23,6 @@ #ifndef NUCLEAR_EXTENSION_IOCONTROLLER_WINDOWS_HPP #define NUCLEAR_EXTENSION_IOCONTROLLER_WINDOWS_HPP -#include "../PowerPlant.hpp" #include "../Reactor.hpp" #include "../dsl/word/IO.hpp" #include "../util/platform.hpp" diff --git a/src/extension/NetworkController.hpp b/src/extension/NetworkController.hpp index 91ea9a948..6123d9ffd 100644 --- a/src/extension/NetworkController.hpp +++ b/src/extension/NetworkController.hpp @@ -27,7 +27,6 @@ #include #include -#include "../PowerPlant.hpp" #include "../Reactor.hpp" #include "../util/get_hostname.hpp" #include "network/NUClearNetwork.hpp" diff --git a/src/extension/network/NUClearNetwork.cpp b/src/extension/network/NUClearNetwork.cpp index b89defc09..bc240d03a 100644 --- a/src/extension/network/NUClearNetwork.cpp +++ b/src/extension/network/NUClearNetwork.cpp @@ -23,14 +23,13 @@ #include "NUClearNetwork.hpp" #include -#include #include -#include +#include +#include #include #include #include -#include "../../util/network/get_interfaces.hpp" #include "../../util/network/if_number_from_address.hpp" #include "../../util/network/resolve.hpp" #include "../../util/platform.hpp" @@ -117,6 +116,8 @@ namespace extension { std::memcpy(key.data(), &address.ipv6.sin6_addr, sizeof(address.ipv6.sin6_addr)); key[8] = address.ipv6.sin6_port; break; + + default: throw std::invalid_argument("Unknown address family"); } return key; diff --git a/src/message/TimeTravel.hpp b/src/message/TimeTravel.hpp index 9a32ed881..49c3819cf 100644 --- a/src/message/TimeTravel.hpp +++ b/src/message/TimeTravel.hpp @@ -34,7 +34,7 @@ namespace message { * to the new time and rate. */ struct TimeTravel { - enum class Action { + enum class Action : uint8_t { /// Adjust clock and move all chrono tasks with it RELATIVE, diff --git a/src/util/FunctionFusion.hpp b/src/util/FunctionFusion.hpp index 5a25e83a2..b86e60388 100644 --- a/src/util/FunctionFusion.hpp +++ b/src/util/FunctionFusion.hpp @@ -48,7 +48,7 @@ namespace util { * @return The object returned by the called subfunction */ template - auto apply_function_fusion_call(std::tuple&& args, + auto apply_function_fusion_call(const std::tuple& args, const Sequence& /*shared*/, const Sequence& /*selected*/) -> decltype(Function::call(std::get(args)..., std::get(args)...)) { @@ -57,18 +57,18 @@ namespace util { /** * Applies a single set of function fusion with argument ranges + * * Calls the function held in the template type Function. - * for the arguments it uses the parameter packs Shared and Selected - * to expand the passed tuple args and forward those selected - * arguments to the function. + * For the arguments it uses the parameter packs Shared and Selected to expand the passed tuple args and forward + * those selected arguments to the function. * - * @param args the arguments that were passed to the superfunction + * @tparam Function The struct that holds the call function wrapper to be called + * @tparam Shared The number of parameters (from 0) to use in the call + * @tparam Start The index of the first argument to pass to the function + * @tparam End The index of the element after the last argument to pass to the function + * @tparam Arguments The types of the arguments passed into the function * - * @tparam Function the struct that holds the call function wrapper to be called - * @tparam Shared the number of parameters (from 0) to use in the call - * @tparam Start the index of the first argument to pass to the function - * @tparam End the index of the element after the last argument to pass to the function - * @tparam Arguments the types of the arguments passed into the function + * @param args the arguments that were passed to the super-function * * @return the object returned by the called subfunction */ @@ -95,6 +95,8 @@ namespace util { */ template struct FunctionFusionCaller, Shared, std::tuple<>, std::tuple> { + // This function is just here to satisfy the templates + // NOLINTNEXTLINE(cppcoreguidelines-rvalue-reference-param-not-moved) static std::tuple<> call(Arguments&&... /*args*/) { return {}; } @@ -137,6 +139,8 @@ namespace util { * @return the result of calling this specific function */ template + // It is forwarded as a tuple + // NOLINTNEXTLINE(cppcoreguidelines-rvalue-reference-param-not-moved) static auto call_one(const Sequence& /*e*/, Arguments&&... args) -> decltype(apply_function_fusion_call(std::forward_as_tuple(args...))) { @@ -153,7 +157,7 @@ namespace util { * @return ignore */ template - static inline bool call_one(...); + static bool call_one(...); /// The FunctionFusionCaller next step in the recursion using NextStep = diff --git a/src/util/network/get_interfaces.cpp b/src/util/network/get_interfaces.cpp index 06e8297bc..fc1009556 100644 --- a/src/util/network/get_interfaces.cpp +++ b/src/util/network/get_interfaces.cpp @@ -22,9 +22,9 @@ #include "get_interfaces.hpp" -#include #include #include +#include #include "../platform.hpp" @@ -177,6 +177,7 @@ namespace util { switch (it->ifa_addr->sa_family) { case AF_INET: std::memcpy(&iface.netmask, it->ifa_netmask, sizeof(sockaddr_in)); break; case AF_INET6: std::memcpy(&iface.netmask, it->ifa_netmask, sizeof(sockaddr_in6)); break; + default: break; // We don't care about other address families } } @@ -184,6 +185,7 @@ namespace util { switch (it->ifa_addr->sa_family) { case AF_INET: std::memcpy(&iface.broadcast, it->ifa_dstaddr, sizeof(sockaddr_in)); break; case AF_INET6: std::memcpy(&iface.broadcast, it->ifa_dstaddr, sizeof(sockaddr_in6)); break; + default: break; // We don't care about other address families } } diff --git a/tests/networktest.cpp b/tests/networktest.cpp index 0bebf9190..340e67d6c 100644 --- a/tests/networktest.cpp +++ b/tests/networktest.cpp @@ -168,6 +168,7 @@ class TestReactor : public NUClear::Reactor { // NOLINTNEXTLINE(bugprone-exception-escape) int main(int argc, const char* argv[]) { + // NOLINTNEXTLINE(bugprone-signal-handler,cert-msc54-cpp,cert-sig30-c) auto old_sigint = signal(SIGINT, [](int /*signal*/) { NUClear::PowerPlant::powerplant->shutdown(); }); if (old_sigint == SIG_ERR) { std::cerr << "Failed to set SIGINT handler"; diff --git a/tests/tests/dsl/Priority.cpp b/tests/tests/dsl/Priority.cpp index 4b760bc76..370f83458 100644 --- a/tests/tests/dsl/Priority.cpp +++ b/tests/tests/dsl/Priority.cpp @@ -75,6 +75,7 @@ class TestReactor : public test_util::TestBase { case 4: on>, Priority::IDLE>().then([] { events.push_back("Idle Message<3>"); }); break; + default: throw std::invalid_argument("Should be impossible"); } } diff --git a/tests/tests/dsl/TCP.cpp b/tests/tests/dsl/TCP.cpp index 584eff92a..a5b3a27ea 100644 --- a/tests/tests/dsl/TCP.cpp +++ b/tests/tests/dsl/TCP.cpp @@ -31,12 +31,12 @@ namespace { /// Events that occur during the test std::vector events; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) -enum TestPorts { +enum TestPorts : in_port_t { KNOWN_V4_PORT = 40010, KNOWN_V6_PORT = 40011, }; -enum TestType { +enum TestType : uint8_t { V4_KNOWN, V4_EPHEMERAL, V6_KNOWN, diff --git a/tests/tests/dsl/UDP.cpp b/tests/tests/dsl/UDP.cpp index 7a3d09683..2d014661a 100644 --- a/tests/tests/dsl/UDP.cpp +++ b/tests/tests/dsl/UDP.cpp @@ -31,7 +31,7 @@ namespace { /// Events that occur during the test std::vector events; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) -enum TestPorts { +enum TestPorts : in_port_t { UNICAST_V4 = 40000, UNICAST_V6 = 40001, BROADCAST_V4 = 40002, @@ -59,7 +59,7 @@ in_port_t broad_v4_port = 0; // NOLINT(cppcoreguidelines-avoid-non-const-global in_port_t multi_v4_port = 0; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) in_port_t multi_v6_port = 0; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) -enum TestType { +enum TestType : uint8_t { UNICAST_V4_KNOWN, UNICAST_V4_EPHEMERAL, UNICAST_V6_KNOWN, @@ -74,7 +74,7 @@ enum TestType { std::vector active_tests; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) inline std::string get_broadcast_addr() { - static std::string addr{}; + static std::string addr; if (!addr.empty()) { return addr; @@ -371,6 +371,7 @@ TEST_CASE("Testing sending and receiving of UDP messages", "[api][network][udp]" NUClear::Configuration config; config.thread_count = 1; NUClear::PowerPlant plant(config); + plant.install(); plant.install(); plant.start(); diff --git a/tests/tests/log/Log.cpp b/tests/tests/log/Log.cpp index f1f7619c7..c0088118b 100644 --- a/tests/tests/log/Log.cpp +++ b/tests/tests/log/Log.cpp @@ -180,7 +180,7 @@ TEST_CASE("Testing the Log<>() function", "[api][log]") { // Test logs from reactions directly for (const auto& log_level : levels) { if (display_level <= log_level) { - const std::string expected = "Direct Reaction " + std::to_string(log_level); + const std::string expected = "Direct Reaction " + NUClear::to_string(log_level); REQUIRE(messages[i].message == expected); REQUIRE(messages[i].level == log_level); REQUIRE(messages[i++].from_reaction); @@ -189,7 +189,7 @@ TEST_CASE("Testing the Log<>() function", "[api][log]") { // Test logs from reactions indirectly for (const auto& log_level : levels) { if (display_level <= log_level) { - const std::string expected = "Indirect Reaction " + std::to_string(log_level); + const std::string expected = "Indirect Reaction " + NUClear::to_string(log_level); REQUIRE(messages[i].message == expected); REQUIRE(messages[i].level == log_level); REQUIRE(messages[i++].from_reaction); @@ -198,7 +198,7 @@ TEST_CASE("Testing the Log<>() function", "[api][log]") { // Test logs from free floating functions for (const auto& log_level : levels) { // No filter here, free floating prints everything - const std::string expected = "Non Reaction " + std::to_string(log_level); + const std::string expected = "Non Reaction " + NUClear::to_string(log_level); REQUIRE(messages[i].message == expected); REQUIRE(messages[i].level == log_level); REQUIRE_FALSE(messages[i++].from_reaction); @@ -207,7 +207,7 @@ TEST_CASE("Testing the Log<>() function", "[api][log]") { // Test post-shutdown logs { - const std::string expected = "Post Powerplant Shutdown " + std::to_string(NUClear::FATAL); + const std::string expected = "Post Powerplant Shutdown " + NUClear::to_string(NUClear::FATAL); REQUIRE(messages[i].message == expected); REQUIRE(messages[i].level == NUClear::FATAL); REQUIRE(messages[i++].from_reaction); @@ -219,7 +219,7 @@ TEST_CASE("Testing the Log<>() function", "[api][log]") { // Test logs from free floating functions for (const auto& log_level : levels) { // No filter here, free floating prints everything - const std::string expected = "Non Reaction " + std::to_string(log_level); + const std::string expected = "Non Reaction " + NUClear::to_string(log_level); REQUIRE(messages[i].message == expected); REQUIRE(messages[i].level == log_level); REQUIRE_FALSE(messages[i++].from_reaction);