Address some issues found by sonarcloud (#86)

.github/workflows/sonarcloud.yaml

@@ -3,6 +3,7 @@ on:
   push:
     branches:
       - main
+      - sonar
   pull_request:
     types: [opened, synchronize, reopened]
 

docs/startup.rst

@@ -22,7 +22,7 @@ file for the process.
 
 .. code-block:: C++
 
-    NUClear::PowerPlant::Configuration config;
+    NUClear::Configuration config;
     config.thread_count = 1;
     NUClear::PowerPlant plant(config);
 

src/CMakeLists.txt

@@ -35,7 +35,7 @@ add_library(NUClear::nuclear ALIAS nuclear)
 # Set compile options for NUClear
 target_link_libraries(nuclear ${CMAKE_THREAD_LIBS_INIT})
 set_target_properties(nuclear PROPERTIES POSITION_INDEPENDENT_CODE ON)
-target_compile_features(nuclear PUBLIC c_std_99 cxx_std_14)
+target_compile_features(nuclear PUBLIC cxx_std_14)
 
 # Enable warnings, and all warnings are errors
 if(MSVC)

src/Configuration.hpp

@@ -0,0 +1,41 @@
+/*
+ * MIT License
+ *
+ * Copyright (c) 2023 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.
+ */
+
+#ifndef NUCLEAR_CONFIGURATION_HPP
+#define NUCLEAR_CONFIGURATION_HPP
+
+#include <cstddef>
+#include <thread>
+
+namespace NUClear {
+
+/**
+ * @brief This class holds the configuration for a PowerPlant.
+ */
+struct Configuration {
+    /// @brief The number of threads the system will use
+    size_t thread_count = std::thread::hardware_concurrency() == 0 ? 2 : std::thread::hardware_concurrency();
+};
+
+}  // namespace NUClear
+
+#endif  // NUCLEAR_CONFIGURATION_HPP

src/PowerPlant.hpp

@@ -37,6 +37,7 @@
 #include <vector>
 
 // Utilities
+#include "Configuration.hpp"
 #include "LogLevel.hpp"
 #include "message/LogMessage.hpp"
 #include "threading/ReactionTask.hpp"
@@ -64,25 +65,6 @@ class PowerPlant {
     friend class Reactor;
 
 public:
-    /**
-     * @brief This class holds the configuration for a PowerPlant.
-     *
-     * @details
-     *  It configures the number of threads that will be in the PowerPlants thread pool
-     */
-    struct Configuration {
-        /// @brief default to the amount of hardware concurrency (or 2) threads
-        Configuration()
-            : thread_count(std::thread::hardware_concurrency() == 0 ? 2 : std::thread::hardware_concurrency()) {}
-
-        /// @brief The number of threads the system will use
-        size_t thread_count;
-    };
-
-    /// @brief Holds the configuration information for this PowerPlant (such as number of pool threads)
-    const Configuration configuration;
-
-
     // There can only be one powerplant, so this is it
     static PowerPlant* powerplant;  // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)
 
@@ -91,9 +73,7 @@ class PowerPlant {
      *  Constructs a PowerPlant with the given configuration and provides access
      *  to argv for all reactors.
      *
-     * @details
-     *  If PowerPlant is constructed with argc and argv then a CommandLineArguments
-     *  message will be emitted and available to all reactors.
+     * @details Arguments passed to this function will be emitted as a CommandLineArguments message.
      *
      * @param config The PowerPlant's configuration
      * @param argc The number of command line arguments

src/PowerPlant.ipp

@@ -27,8 +27,7 @@
 namespace NUClear {
 
 // NOLINTNEXTLINE(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
-inline PowerPlant::PowerPlant(Configuration config, int argc, const char* argv[])
-    : configuration(config), scheduler(config.thread_count) {
+inline PowerPlant::PowerPlant(Configuration config, int argc, const char* argv[]) : scheduler(config.thread_count) {
 
     // Stop people from making more then one powerplant
     if (powerplant != nullptr) {
@@ -151,19 +150,16 @@ void PowerPlant::emit(Arguments&&... args) {
     emit_shared<First, Remainder...>(std::forward<Arguments>(args)...);
 }
 
-// Anonymous metafunction that concatenates everything into a single string
-namespace {
-    template <typename T>
-    inline void log_impl(std::stringstream& output, T&& first) {
-        output << first;
-    }
+template <typename T>
+inline void log_impl(std::stringstream& output, T&& first) {
+    output << std::forward<T>(first);
+}
 
-    template <typename First, typename... Remainder>
-    inline void log_impl(std::stringstream& output, First&& first, Remainder&&... args) {
-        output << first << " ";
-        log_impl(output, std::forward<Remainder>(args)...);
-    }
-}  // namespace
+template <typename First, typename... Remainder>
+inline void log_impl(std::stringstream& output, First&& first, Remainder&&... args) {
+    output << std::forward<First>(first) << " ";
+    log_impl(output, std::forward<Remainder>(args)...);
+}
 
 template <enum LogLevel level, typename... Arguments>
 void PowerPlant::log(Arguments&&... args) {

src/Reactor.hpp

@@ -142,7 +142,7 @@ class Reactor {
 public:
     friend class PowerPlant;
 
-    Reactor(std::unique_ptr<Environment> environment)
+    explicit Reactor(std::unique_ptr<Environment> environment)
         : powerplant(environment->powerplant)
         , reactor_name(environment->reactor_name)
         , log_level(environment->log_level) {}

src/clock.hpp

@@ -27,12 +27,13 @@
 
 namespace NUClear {
 
-#ifndef NUCLEAR_CLOCK_TYPE
-    #define NUCLEAR_CLOCK_TYPE std::chrono::steady_clock
-#endif  // NUCLEAR_CLOCK_TYPE
-
-/// @brief The base clock that is used when defining the NUClear clock
+#ifdef NUCLEAR_CLOCK_TYPE
+/// @brief The custom base clock that is used when defining the NUClear clock
 using base_clock = NUCLEAR_CLOCK_TYPE;
+#else
+/// @brief The default base clock that is used when defining the NUClear clock
+using base_clock = std::chrono::steady_clock;
+#endif  // NUCLEAR_CLOCK_TYPE
 
 #ifndef NUCLEAR_CUSTOM_CLOCK
 

src/dsl/fusion/GroupFusion.hpp

@@ -24,6 +24,7 @@
 #define NUCLEAR_DSL_FUSION_GROUPFUSION_HPP
 
 #include <algorithm>
+#include <stdexcept>
 
 #include "../../threading/Reaction.hpp"
 #include "../operation/DSLProxy.hpp"
@@ -88,8 +89,8 @@ namespace dsl {
         struct GroupFuser<std::tuple<Word1, Word2, WordN...>> {
 
             template <typename DSL>
-            static inline void group(threading::Reaction& /*reaction*/) {
-                throw std::runtime_error("Can not be a member of more than one group");
+            static inline void group(const threading::Reaction& /*reaction*/) {
+                throw std::invalid_argument("Can not be a member of more than one group");
             }
         };
 

src/dsl/fusion/NoOp.hpp

@@ -42,35 +42,39 @@ namespace dsl {
         struct NoOp {
 
             template <typename DSL, typename... Args>
-            static inline void bind(const std::shared_ptr<threading::Reaction>& /*reaction*/, Args... /*args*/) {}
+            static inline void bind(const std::shared_ptr<threading::Reaction>& /*reaction*/, Args... /*args*/) {
+                // Empty as this is a no-op placeholder
+            }
 
             template <typename DSL>
-            static inline std::tuple<> get(threading::Reaction& /*reaction*/) {
+            static inline std::tuple<> get(const threading::Reaction& /*reaction*/) {
                 return {};
             }
 
             template <typename DSL>
-            static inline bool precondition(threading::Reaction& /*reaction*/) {
+            static inline bool precondition(const threading::Reaction& /*reaction*/) {
                 return true;
             }
 
             template <typename DSL>
-            static inline int priority(threading::Reaction& /*reaction*/) {
+            static inline int priority(const threading::Reaction& /*reaction*/) {
                 return word::Priority::NORMAL::value;
             }
 
             template <typename DSL>
-            static inline util::GroupDescriptor group(threading::Reaction& /*reaction*/) {
+            static inline util::GroupDescriptor group(const threading::Reaction& /*reaction*/) {
                 return util::GroupDescriptor{};
             }
 
             template <typename DSL>
-            static inline util::ThreadPoolDescriptor pool(threading::Reaction& /*reaction*/) {
+            static inline util::ThreadPoolDescriptor pool(const threading::Reaction& /*reaction*/) {
                 return util::ThreadPoolDescriptor{};
             }
 
             template <typename DSL>
-            static inline void postcondition(threading::ReactionTask& /*task*/) {}
+            static inline void postcondition(const threading::ReactionTask& /*task*/) {
+                // Empty as this is a no-op placeholder
+            }
         };
 
         /**
@@ -80,19 +84,19 @@ namespace dsl {
         struct ParsedNoOp {
             struct DSL {};
 
-            static inline std::tuple<> bind(const std::shared_ptr<threading::Reaction>&);
+            static std::tuple<> bind(const std::shared_ptr<threading::Reaction>&);
 
-            static inline std::tuple<> get(threading::Reaction&);
+            static std::tuple<> get(threading::Reaction&);
 
-            static inline bool precondition(threading::Reaction&);
+            static bool precondition(threading::Reaction&);
 
-            static inline int priority(threading::Reaction&);
+            static int priority(threading::Reaction&);
 
-            static inline util::GroupDescriptor group(threading::Reaction&);
+            static util::GroupDescriptor group(threading::Reaction&);
 
-            static inline util::ThreadPoolDescriptor pool(threading::Reaction&);
+            static util::ThreadPoolDescriptor pool(threading::Reaction&);
 
-            static inline void postcondition(threading::ReactionTask&);
+            static void postcondition(threading::ReactionTask&);
         };
 
     }  // namespace fusion

src/dsl/fusion/PoolFusion.hpp

@@ -24,6 +24,7 @@
 #define NUCLEAR_DSL_FUSION_POOLFUSION_HPP
 
 #include <algorithm>
+#include <stdexcept>
 
 #include "../../threading/Reaction.hpp"
 #include "../operation/DSLProxy.hpp"
@@ -88,8 +89,8 @@ namespace dsl {
         struct PoolFuser<std::tuple<Word1, Word2, WordN...>> {
 
             template <typename DSL>
-            static inline util::ThreadPoolDescriptor pool(threading::Reaction& /*reaction*/) {
-                throw std::runtime_error("Can not be a member of more than one pool");
+            static inline util::ThreadPoolDescriptor pool(const threading::Reaction& /*reaction*/) {
+                throw std::invalid_argument("Can not be a member of more than one pool");
             }
         };
 

src/dsl/operation/CacheGet.hpp

@@ -42,7 +42,7 @@ namespace dsl {
         struct CacheGet {
 
             template <typename DSL, typename T = DataType>
-            static inline std::shared_ptr<const T> get(threading::Reaction& /*reaction*/) {
+            static inline std::shared_ptr<const T> get(const threading::Reaction& /*reaction*/) {
 
                 return store::ThreadStore<std::shared_ptr<T>>::value == nullptr
                            ? store::DataStore<DataType>::get()

src/dsl/operation/ChronoTask.hpp

@@ -51,7 +51,7 @@ namespace dsl {
             ChronoTask(std::function<bool(NUClear::clock::time_point&)>&& task,
                        NUClear::clock::time_point time,
                        uint64_t id)
-                : task(task), time(time), id(id) {}
+                : task(std::move(task)), time(time), id(id) {}
 
             /**
              * @brief Run the task and return true if the time has been updated to run again

src/dsl/operation/TypeBind.hpp

@@ -46,17 +46,17 @@ namespace dsl {
             static inline void bind(const std::shared_ptr<threading::Reaction>& reaction) {
 
                 // Our unbinder to remove this reaction
-                reaction->unbinders.push_back([](threading::Reaction& reaction) {
+                reaction->unbinders.push_back([](const threading::Reaction& r) {
                     auto& vec = store::TypeCallbackStore<DataType>::get();
 
-                    auto item = std::find_if(
+                    auto it = std::find_if(
                         std::begin(vec),
                         std::end(vec),
-                        [&reaction](const std::shared_ptr<threading::Reaction>& r) { return r->id == reaction.id; });
+                        [&r](const std::shared_ptr<threading::Reaction>& item) { return item->id == r.id; });
 
                     // If the item is in the list erase the item
-                    if (item != std::end(vec)) {
-                        vec.erase(item);
+                    if (it != std::end(vec)) {
+                        vec.erase(it);
                     }
                 });
 
@@ -75,17 +75,17 @@ namespace dsl {
                 reaction->emit_stats = false;
 
                 // Our unbinder to remove this reaction
-                reaction->unbinders.push_back([](threading::Reaction& r) {
+                reaction->unbinders.push_back([](const threading::Reaction& r) {
                     auto& vec = store::TypeCallbackStore<message::ReactionStatistics>::get();
 
-                    auto item = std::find_if(
+                    auto it = std::find_if(
                         std::begin(vec),
                         std::end(vec),
                         [&r](const std::shared_ptr<threading::Reaction>& item) { return item->id == r.id; });
 
                     // If the item is in the list erase the item
-                    if (item != std::end(vec)) {
-                        vec.erase(item);
+                    if (it != std::end(vec)) {
+                        vec.erase(it);
                     }
                 });
 

src/dsl/operation/Unbind.hpp

@@ -38,7 +38,7 @@ namespace dsl {
          */
         template <typename Word>
         struct Unbind {
-            Unbind(uint64_t id) : id(id){};
+            explicit Unbind(const uint64_t& id) : id(id){};
 
             /// The id of the task to unbind
             const uint64_t id{0};

src/dsl/word/Always.hpp

@@ -72,7 +72,7 @@ namespace dsl {
         struct Always {
 
             template <typename DSL>
-            static inline util::ThreadPoolDescriptor pool(threading::Reaction& reaction) {
+            static inline util::ThreadPoolDescriptor pool(const threading::Reaction& reaction) {
                 static std::map<uint64_t, uint64_t> pool_id;
                 static std::mutex mutex;
 
@@ -112,13 +112,13 @@ namespace dsl {
                                                    always_reaction->identifiers.reactor,
                                                    always_reaction->identifiers.dsl,
                                                    always_reaction->identifiers.function},
-                    [always_reaction](threading::Reaction& idle_reaction) -> util::GeneratedCallback {
-                        auto callback = [&idle_reaction, always_reaction](threading::ReactionTask& /*task*/) {
+                    [always_reaction](threading::Reaction& ir) -> util::GeneratedCallback {
+                        auto callback = [&ir, always_reaction](const threading::ReactionTask& /*task*/) {
                             // Get a task for the always reaction and submit it to the scheduler
                             always_reaction->reactor.powerplant.submit(always_reaction->get_task());
 
                             // Get a task for the idle reaction and submit it to the scheduler
-                            idle_reaction.reactor.powerplant.submit(idle_reaction.get_task());
+                            ir.reactor.powerplant.submit(ir.get_task());
                         };
 
                         // Make sure that idle reaction always has lower priority than the always reaction

src/dsl/word/Buffer.hpp

@@ -49,7 +49,7 @@ namespace dsl {
         struct Buffer {
 
             template <typename DSL>
-            static inline bool precondition(threading::Reaction& reaction) {
+            static inline bool precondition(const threading::Reaction& reaction) {
                 // We only run if there are less than the target number of active tasks
                 return reaction.active_tasks < (n + 1);
             }