Various clang tidy recommendations (#227)

hikinggrass · web-flow · commit 78c1de0ec451 · 2025-01-08T08:28:50.000+01:00
* Add performance checks to clang-tidy config
* Use const ref for a few more variables and parameters
* Add some moves that clang-tidy suggested
* Fix widening conversion warning by multiplying std::size_t with int
* Fix narrowing conversion warning in yaml error handler by clamping maximum length
* Pass VersionInfo by value and move it like ModuleCallbacks
* Don't use loop for local_handlers copy

---------

Signed-off-by: Kai-Uwe Hermann &lt;kai-uwe.hermann@pionix.de&gt;
diff --git a/.clang-tidy b/.clang-tidy
@@ -1,6 +1,8 @@
 Checks: >
     bugprone*,
     misc-const-correctness,
+    performance*,
+    -performance-avoid-endl,
     -llvmlibc*,
     -fuchsia-overloaded-operator,
     -fuchsia-statically-constructed-objects,
@@ -18,3 +20,5 @@ Checks: >
     -cppcoreguidelines-non-private-member-variables-in-classes,
     -bugprone-easily-swappable-parameters
 HeaderFilterRegex:  ".*"
+CheckOptions:
+  - { key: performance-unnecessary-value-param.AllowedTypes, value: ((std::shared_ptr)) }
diff --git a/everestjs/everestjs.cpp b/everestjs/everestjs.cpp
@@ -612,10 +612,10 @@ static Napi::Value boot_module(const Napi::CallbackInfo& info) {
                                 "Module with identifier '" << module_id << "' not found in config!"));
         }
 
-        const std::string& module_name = config->get_main_config()[module_id]["module"].get<std::string>();
-        auto module_manifest = config->get_manifests()[module_name];
+        const std::string& module_name = config->get_module_name(module_id);
+        const auto& module_manifest = config->get_manifests().at(module_name);
         // FIXME (aw): get_classes should be called get_units and should contain the type of class for each unit
-        auto module_impls = config->get_interfaces()[module_name];
+        const auto& module_impls = config->get_interfaces().at(module_name);
 
         // initialize everest framework
         const auto& module_identifier = config->printable_identifier(module_id);
@@ -733,9 +733,9 @@ static Napi::Value boot_module(const Napi::CallbackInfo& info) {
         auto uses_list_reqs_prop = Napi::Object::New(env);
         auto uses_cmds_prop = Napi::Object::New(env);
         auto uses_list_cmds_prop = Napi::Object::New(env);
-        for (auto& requirement : module_manifest["requires"].items()) {
+        for (const auto& requirement : module_manifest.at("requires").items()) {
             auto req_prop = Napi::Object::New(env);
-            auto const& requirement_id = requirement.key();
+            const auto& requirement_id = requirement.key();
             json req_route_list = config->resolve_requirement(module_id, requirement_id);
             // if this was a requirement with min_connections == 1 and max_connections == 1,
             // this will be simply a single connection, but an array of connections otherwise
@@ -747,13 +747,13 @@ static Napi::Value boot_module(const Napi::CallbackInfo& info) {
             auto req_array_prop = Napi::Array::New(env);
             auto req_mod_cmds_array = Napi::Array::New(env);
             for (std::size_t i = 0; i < req_route_list.size(); i++) {
-                auto req_route = req_route_list[i];
+                const auto& req_route = req_route_list[i];
                 const std::string& requirement_module_id = req_route["module_id"];
                 const std::string& requirement_impl_id = req_route["implementation_id"];
                 // FIXME (aw): why is const auto& not possible for the following line?
                 // we only want cmds/vars from the required interface to be usable, not from it's child interfaces
-                std::string interface_name = req_route["required_interface"].get<std::string>();
-                auto requirement_impl_intf = config->get_interface_definition(interface_name);
+                const std::string& interface_name = req_route["required_interface"].get<std::string>();
+                const auto& requirement_impl_intf = config->get_interface_definition(interface_name);
                 auto requirement_vars = Everest::Config::keys(requirement_impl_intf["vars"]);
                 auto requirement_cmds = Everest::Config::keys(requirement_impl_intf["cmds"]);
 
diff --git a/everestpy/src/everest/everestpy.cpp b/everestpy/src/everest/everestpy.cpp
@@ -130,7 +130,7 @@ PYBIND11_MODULE(everestpy, m) {
         .def(py::init<const std::string&, const RuntimeSession&>())
         .def("say_hello", &Module::say_hello)
         .def("init_done", py::overload_cast<>(&Module::init_done))
-        .def("init_done", py::overload_cast<std::function<void()>>(&Module::init_done))
+        .def("init_done", py::overload_cast<const std::function<void()>&>(&Module::init_done))
         .def("call_command", &Module::call_command)
         .def("publish_variable", &Module::publish_variable)
         .def("implement_command", &Module::implement_command)
diff --git a/everestpy/src/everest/misc.cpp b/everestpy/src/everest/misc.cpp
@@ -82,8 +82,8 @@ RuntimeSession::RuntimeSession() {
 ModuleSetup create_setup_from_config(const std::string& module_id, Everest::Config& config) {
     ModuleSetup setup;
 
-    const std::string& module_name = config.get_main_config().at(module_id).at("module");
-    const auto module_manifest = config.get_manifests().at(module_name);
+    const std::string& module_name = config.get_module_name(module_id);
+    const auto& module_manifest = config.get_manifests().at(module_name);
 
     // setup connections
     for (const auto& requirement : module_manifest.at("requires").items()) {
@@ -107,7 +107,7 @@ ModuleSetup create_setup_from_config(const std::string& module_id, Everest::Conf
             const auto& req_route = req_route_list[i];
             const auto fulfillment =
                 Fulfillment{req_route["module_id"], req_route["implementation_id"], {requirement_id, i}};
-            fulfillment_list.emplace_back(std::move(fulfillment));
+            fulfillment_list.emplace_back(fulfillment);
         }
     }
 
diff --git a/everestpy/src/everest/module.hpp b/everestpy/src/everest/module.hpp
@@ -21,11 +21,11 @@ class Module {
 
     ModuleSetup say_hello();
 
-    void init_done(std::function<void()> on_ready_handler) {
+    void init_done(const std::function<void()>& on_ready_handler) {
         this->handle->check_code();
 
         if (on_ready_handler) {
-            handle->register_on_ready_handler(std::move(on_ready_handler));
+            handle->register_on_ready_handler(on_ready_handler);
         }
 
         handle->signal_ready();
diff --git a/include/framework/everest.hpp b/include/framework/everest.hpp
@@ -66,7 +66,7 @@ class Everest {
     ///
     /// \brief Allows a module to indicate that it provides the given command \p cmd
     ///
-    void provide_cmd(const std::string impl_id, const std::string cmd_name, const JsonCommand handler);
+    void provide_cmd(const std::string& impl_id, const std::string& cmd_name, const JsonCommand& handler);
     void provide_cmd(const cmd& cmd);
 
     ///
@@ -217,7 +217,7 @@ class Everest {
     bool telemetry_enabled;
     std::optional<ModuleTierMappings> module_tier_mappings;
 
-    void handle_ready(nlohmann::json data);
+    void handle_ready(const nlohmann::json& data);
 
     void heartbeat();
 
diff --git a/include/framework/runtime.hpp b/include/framework/runtime.hpp
@@ -192,9 +192,9 @@ class ModuleLoader {
 
 public:
     explicit ModuleLoader(int argc, char* argv[], ModuleCallbacks callbacks) :
-        ModuleLoader(argc, argv, callbacks, {"undefined project", "undefined version", "undefined git version"}){};
-    explicit ModuleLoader(int argc, char* argv[], ModuleCallbacks callbacks,
-                          const VersionInformation version_information);
+        ModuleLoader(argc, argv, std::move(callbacks),
+                     {"undefined project", "undefined version", "undefined git version"}){};
+    explicit ModuleLoader(int argc, char* argv[], ModuleCallbacks callbacks, VersionInformation version_information);
 
     int initialize();
 };
diff --git a/include/utils/helpers.hpp b/include/utils/helpers.hpp
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright Pionix GmbH and Contributors to EVerest
+
+#pragma once
+
+#include <limits>
+
+#include <utils/types.hpp>
+
+namespace Everest::helpers {
+template <typename T, typename U> T constexpr clamp_to(U len) {
+    return (len <= std::numeric_limits<T>::max()) ? static_cast<T>(len) : std::numeric_limits<T>::max();
+}
+} // namespace Everest::helpers
diff --git a/include/utils/mqtt_abstraction_impl.hpp b/include/utils/mqtt_abstraction_impl.hpp
@@ -20,7 +20,7 @@
 
 #include <utils/thread.hpp>
 
-constexpr std::size_t MQTT_BUF_SIZE = 500 * 1024;
+constexpr auto MQTT_BUF_SIZE = 500 * std::size_t{1024};
 
 namespace Everest {
 /// \brief Contains a payload and the topic it was received on with additional QOS
diff --git a/lib/config.cpp b/lib/config.cpp
@@ -78,8 +78,8 @@ static ParsedConfigMap parse_config_map(const json& config_map_schema, const jso
 
     // validate each config entry
     for (const auto& config_entry_el : config_map_schema.items()) {
-        const std::string config_entry_name = config_entry_el.key();
-        const json config_entry = config_entry_el.value();
+        const std::string& config_entry_name = config_entry_el.key();
+        const json& config_entry = config_entry_el.value();
 
         // only convenience exception, would be catched by schema validation below if not thrown here
         if (!config_entry.contains("default") and !config_map.contains(config_entry_name)) {
@@ -125,7 +125,7 @@ static auto get_provides_for_probe_module(const std::string& probe_module_id, co
         const auto& connections = module_config.value("connections", json::object());
 
         for (const auto& connection : connections.items()) {
-            const std::string req_id = connection.key();
+            const std::string& req_id = connection.key();
             const std::string module_name = module_config.at("module");
             const auto& module_manifest = manifests.at(module_name);
 
@@ -161,7 +161,7 @@ static auto get_provides_for_probe_module(const std::string& probe_module_id, co
 
 static auto get_requirements_for_probe_module(const std::string& probe_module_id, const json& config,
                                               const json& manifests) {
-    const auto probe_module_config = config.at(probe_module_id);
+    const auto& probe_module_config = config.at(probe_module_id);
 
     auto requirements = json::object();
 
@@ -181,22 +181,23 @@ static auto get_requirements_for_probe_module(const std::string& probe_module_id
 
             if (module_config_it == config.end()) {
                 EVLOG_AND_THROW(
-                    EverestConfigError("ProbeModule refers to a non-existent module id '" + module_id + "'"));
+                    EverestConfigError(fmt::format("ProbeModule refers to a non-existent module id '{}'", module_id)));
             }
 
             const auto& module_manifest = manifests.at(module_config_it->at("module"));
 
             const auto& module_provides_it = module_manifest.find("provides");
 
             if (module_provides_it == module_manifest.end()) {
-                EVLOG_AND_THROW(EverestConfigError("ProbeModule requires something from module id' " + module_id +
-                                                   "', but it does not provide anything"));
+                EVLOG_AND_THROW(EverestConfigError(fmt::format(
+                    "ProbeModule requires something from module id '{}' but it does not provide anything", module_id)));
             }
 
             const auto& provide_it = module_provides_it->find(impl_id);
             if (provide_it == module_provides_it->end()) {
-                EVLOG_AND_THROW(EverestConfigError("ProbeModule requires something from module id '" + module_id +
-                                                   "', but it does not provide '" + impl_id + "'"));
+                EVLOG_AND_THROW(EverestConfigError(
+                    fmt::format("ProbeModule requires something from module id '{}', but it does not provide '{}'",
+                                module_id, impl_id)));
             }
 
             const std::string interface = provide_it->at("interface");
@@ -269,7 +270,7 @@ std::string create_printable_identifier(const ImplementationInfo& info, const st
     BOOST_LOG_FUNCTION();
 
     // no implementation id yet so only return this kind of string:
-    const auto module_string = fmt::format("{}:{}", info.module_id, info.module_name);
+    auto module_string = fmt::format("{}:{}", info.module_id, info.module_name);
     if (impl_id.empty()) {
         return module_string;
     }
@@ -579,7 +580,7 @@ void ManagerConfig::load_and_validate_manifest(const std::string& module_id, con
 
     // validate config for !module
     {
-        const json config_map = module_config.at("config_module");
+        const json& config_map = module_config.at("config_module");
         const json config_map_schema = this->manifests[module_config.at("module").get<std::string>()]["config"];
 
         try {
@@ -887,7 +888,7 @@ void ManagerConfig::resolve_all_requirements() {
 }
 
 void ManagerConfig::parse(json config) {
-    this->main = config;
+    this->main = std::move(config);
     // load type files
     if (this->ms.runtime_settings->validate_schema) {
         int64_t total_time_validation_ms = 0, total_time_parsing_ms = 0;
@@ -1167,7 +1168,7 @@ ModuleConfigs Config::get_module_configs(const std::string& module_id) const {
             for (const auto& entry : conf_map.value().items()) {
                 const json entry_type = config_schema.at(entry.key()).at("type");
                 ConfigEntry value;
-                const json data = entry.value();
+                const json& data = entry.value();
 
                 if (data.is_string()) {
                     value = data.get<std::string>();
@@ -1239,7 +1240,7 @@ void Config::ref_loader(const json_uri& uri, json& schema) {
         schema = nlohmann::json_schema::draft7_schema_builtin;
         return;
     } else {
-        const auto path = uri.path();
+        const auto& path = uri.path();
         if (this->types.contains(path)) {
             schema = this->types[path];
             EVLOG_verbose << fmt::format("ref path \"{}\" schema has been found.", path);
diff --git a/lib/everest.cpp b/lib/everest.cpp
@@ -71,7 +71,7 @@ Everest::Everest(std::string module_id_, const Config& config_, bool validate_da
     // setup error_manager_req_global if enabled + error_database + error_state_monitor
     if (this->module_manifest.contains("enable_global_errors") &&
         this->module_manifest.at("enable_global_errors").get<bool>()) {
-        std::shared_ptr<error::ErrorDatabaseMap> global_error_database = std::make_shared<error::ErrorDatabaseMap>();
+        auto global_error_database = std::make_shared<error::ErrorDatabaseMap>();
         const error::ErrorManagerReqGlobal::SubscribeGlobalAllErrorsFunc subscribe_global_all_errors_func =
             [this](const error::ErrorCallback& callback, const error::ErrorCallback& clear_callback) {
                 this->subscribe_global_all_errors(callback, clear_callback);
@@ -90,7 +90,7 @@ Everest::Everest(std::string module_id_, const Config& config_, bool validate_da
     // setup error_managers, error_state_monitors, error_factories and error_databases for all implementations
     for (const std::string& impl : Config::keys(this->module_manifest.at("provides"))) {
         // setup shared database
-        std::shared_ptr<error::ErrorDatabaseMap> error_database = std::make_shared<error::ErrorDatabaseMap>();
+        auto error_database = std::make_shared<error::ErrorDatabaseMap>();
 
         // setup error manager
         const std::string interface_name = this->module_manifest.at("provides").at(impl).at("interface");
@@ -188,7 +188,7 @@ Everest::Everest(std::string module_id_, const Config& config_, bool validate_da
     }
 
     // register handler for global ready signal
-    const auto handle_ready_wrapper = [this](const std::string&, json data) { this->handle_ready(data); };
+    const auto handle_ready_wrapper = [this](const std::string&, const json& data) { this->handle_ready(data); };
     const auto everest_ready =
         std::make_shared<TypedHandler>(HandlerType::ExternalMQTT, std::make_shared<Handler>(handle_ready_wrapper));
     this->mqtt_abstraction->register_handler(fmt::format("{}ready", mqtt_everest_prefix), everest_ready, QOS::QOS2);
@@ -265,7 +265,7 @@ void Everest::check_code() {
         this->config.get_manifests()[this->config.get_main_config()[this->module_id]["module"].get<std::string>()];
     for (const auto& element : module_manifest.at("provides").items()) {
         const auto& impl_id = element.key();
-        const auto impl_manifest = element.value();
+        const auto& impl_manifest = element.value();
         const auto interface_definition = this->config.get_interface_definition(impl_manifest.at("interface"));
 
         std::set<std::string> cmds_not_registered;
@@ -666,13 +666,13 @@ void Everest::subscribe_global_all_errors(const error::ErrorCallback& callback,
     for (const auto& [module_id, module_name] : this->config.get_module_names()) {
         const json provides = this->config.get_manifests().at(module_name).at("provides");
         for (const auto& impl : provides.items()) {
-            const std::string impl_id = impl.key();
-            const std::string interface = impl.value().at("interface");
+            const std::string& impl_id = impl.key();
+            const std::string& interface = impl.value().at("interface");
             const json errors = this->config.get_interface_definition(interface).at("errors");
             for (const auto& error_namespace_it : errors.items()) {
-                const std::string error_type_namespace = error_namespace_it.key();
+                const std::string& error_type_namespace = error_namespace_it.key();
                 for (const auto& error_name_it : error_namespace_it.value().items()) {
-                    const std::string error_type_name = error_name_it.key();
+                    const std::string& error_type_name = error_name_it.key();
                     const std::string raise_topic =
                         fmt::format("{}/error/{}/{}", this->config.mqtt_prefix(module_id, impl_id),
                                     error_type_namespace, error_type_name);
@@ -784,7 +784,7 @@ void Everest::signal_ready() {
 /// \brief Ready handler for global readyness (e.g. all modules are ready now).
 /// This will called when receiving the global ready signal from manager.
 ///
-void Everest::handle_ready(json data) {
+void Everest::handle_ready(const json& data) {
     BOOST_LOG_FUNCTION();
 
     EVLOG_debug << fmt::format("handle_ready: {}", data.dump());
@@ -818,7 +818,7 @@ void Everest::handle_ready(json data) {
     // this->heartbeat_thread = std::thread(&Everest::heartbeat, this);
 }
 
-void Everest::provide_cmd(const std::string impl_id, const std::string cmd_name, const JsonCommand handler) {
+void Everest::provide_cmd(const std::string& impl_id, const std::string& cmd_name, const JsonCommand& handler) {
     BOOST_LOG_FUNCTION();
 
     // extract manifest definition of this command
@@ -975,16 +975,16 @@ void Everest::provide_cmd(const cmd& cmd) {
         // call cmd handlers (handle async or normal handlers being both:
         // methods or functions)
         // FIXME (aw): this behaviour needs to be checked, i.e. how to distinguish in json between no value and null?
-        return handler(data).value_or(nullptr);
+        return handler(std::move(data)).value_or(nullptr);
     });
 }
 
 json Everest::get_cmd_definition(const std::string& module_id, const std::string& impl_id, const std::string& cmd_name,
                                  bool is_call) {
     BOOST_LOG_FUNCTION();
 
-    const std::string module_name = this->config.get_module_name(module_id);
-    const auto cmds = this->config.get_module_cmds(module_name, impl_id);
+    const auto& module_name = this->config.get_module_name(module_id);
+    const auto& cmds = this->config.get_module_cmds(module_name, impl_id);
 
     if (!this->config.module_provides(module_name, impl_id)) {
         if (!is_call) {
diff --git a/lib/message_queue.cpp b/lib/message_queue.cpp
@@ -70,9 +70,7 @@ MessageHandler::MessageHandler() : running(true) {
             std::vector<std::shared_ptr<TypedHandler>> local_handlers;
             {
                 const std::lock_guard<std::mutex> handlers_lock(handler_list_mutex);
-                for (auto handler : this->handlers) {
-                    local_handlers.push_back(handler);
-                }
+                local_handlers = {std::begin(this->handlers), std::end(this->handlers)};
             }
 
             // distribute this message to the registered handlers
diff --git a/lib/runtime.cpp b/lib/runtime.cpp
diff --git a/lib/types.cpp b/lib/types.cpp
diff --git a/lib/yaml_loader.cpp b/lib/yaml_loader.cpp
diff --git a/src/controller/command_api.cpp b/src/controller/command_api.cpp
diff --git a/src/manager.cpp b/src/manager.cpp
