add clang tidy and fix warnings

Author: facontidavide

.clang-tidy

@@ -0,0 +1,44 @@
+Checks: [
+  "-*",
+  "bugprone-*",
+  "cert-*",
+  "clang-analyzer-*",
+  "concurrency-*",
+  "cppcoreguidelines-*",
+  "misc-*",
+  "modernize-*",
+  "performance-*",
+  "portability-*",
+  "readability-*",
+  "-bugprone-easily-swappable-parameters",
+  "-bugprone-narrowing-conversions",
+  "-cppcoreguidelines-avoid-c-arrays",
+  "-cppcoreguidelines-avoid-magic-numbers",
+  "-cppcoreguidelines-avoid-non-const-global-variables",
+  "-cppcoreguidelines-pro-bounds-array-to-pointer-decay",
+  "-cppcoreguidelines-pro-bounds-constant-array-index",
+  "-cppcoreguidelines-pro-bounds-pointer-arithmetic",
+  "-cppcoreguidelines-pro-type-union-access",
+  "-cppcoreguidelines-pro-type-vararg",
+  "-misc-no-recursion"
+]
+
+
+WarningsAsErrors: '-*,bugprone-*,cert-*,clang-analyzer-*,concurrency-*,cppcoreguidelines-*,misc-*,portability-*,readability-implicit-bool-conversion,-concurrency-mt-unsafe,-readability-function-cognitive-complexity'
+
+CheckOptions:
+  # ignore macros when computing the cyclomatic complexity. problem caused by RCLCPP LOG macros
+  - key:             readability-function-cognitive-complexity.IgnoreMacros
+    value:           'true'
+
+  # This change makes it compatible with MISRA:2023 rule 4.14.1
+  - key:             misc-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic
+    value:           'true'
+
+  # Making a copy of a shared_ptr has a non-zero cost, but this cost is small.
+  # Unfortunately the ROS API (subscriber callbacks) oblige the user to use callbacks functions that will trigger this warning
+  # This is the reason wht the warning is silenced here
+  - key:              performance-unnecessary-value-param.AllowedTypes
+    value:            'std::shared_ptr'
+
+  # Reference: https://clang.llvm.org/extra/clang-tidy/checks/readability/identifier-naming.html

.gitignore

@@ -18,3 +18,6 @@ CMakeSettings.json
 CMakeUserPresets.json
 
 tags
+/clang_tidy_output.log
+/.clang-tidy-venv/*
+/llvm.sh

.pre-commit-config.yaml

@@ -43,6 +43,15 @@ repos:
       - id: clang-format
         args: ['-fallback-style=none', '-i']
 
+  # C++ static analysis (installs clang-tidy automatically via pip)
+  # - repo: https://github.com/mxmlnrdr/clang_tidy_hook
+  #   rev: v0.3.1
+  #   hooks:
+  #     - id: clang-tidy
+  #       args:
+  #         - --config-file=.clang-tidy
+  #         - -p=build
+
   # Spell check
   - repo: https://github.com/codespell-project/codespell
     rev: v2.4.1

include/behaviortree_cpp/actions/pop_from_queue.hpp

@@ -71,18 +71,12 @@ class PopFromQueue : public SyncActionNode
       {
         return NodeStatus::FAILURE;
       }
-      else
-      {
-        T val = items.front();
-        items.pop_front();
-        setOutput("popped_item", val);
-        return NodeStatus::SUCCESS;
-      }
-    }
-    else
-    {
-      return NodeStatus::FAILURE;
+      T val = items.front();
+      items.pop_front();
+      setOutput("popped_item", val);
+      return NodeStatus::SUCCESS;
     }
+    return NodeStatus::FAILURE;
   }
 
   static PortsList providedPorts()
@@ -125,11 +119,8 @@ class QueueSize : public SyncActionNode
       {
         return NodeStatus::FAILURE;
       }
-      else
-      {
-        setOutput("size", int(items.size()));
-        return NodeStatus::SUCCESS;
-      }
+      setOutput("size", int(items.size()));
+      return NodeStatus::SUCCESS;
     }
     return NodeStatus::FAILURE;
   }

include/behaviortree_cpp/actions/sleep_node.h

@@ -35,7 +35,7 @@ class SleepNode : public StatefulActionNode
 
 private:
   TimerQueue<> timer_;
-  uint64_t timer_id_;
+  uint64_t timer_id_ = 0;
 
   std::atomic_bool timer_waiting_ = false;
   std::mutex delay_mutex_;

include/behaviortree_cpp/utils/convert_impl.hpp

@@ -154,12 +154,19 @@ void convertNumber(const SRC& source, DST& target)
       throw std::runtime_error("Value is negative and can't be converted to unsigned");
     }
   }
-  // these conversions are always safe:
+  // these conversions are always safe (no check needed):
   // - same type
   // - float -> double
-  if constexpr(is_same<SRC, DST>() || (is_same<SRC, float>() && is_same<DST, double>()))
+  // - floating point to bool (C-style: any non-zero is true)
+  if constexpr(is_same<SRC, DST>() || (is_same<SRC, float>() && is_same<DST, double>()) ||
+               (std::is_floating_point<SRC>::value && is_same<DST, bool>()))
   {
-    // No check needed
+    target = static_cast<DST>(source);
+  }
+  // integer to bool: only 0 and 1 are valid
+  else if constexpr(is_integer<SRC>() && is_same<DST, bool>())
+  {
+    checkLowerLimit<SRC, DST>(source);
     target = static_cast<DST>(source);
   }
   else if constexpr(both_integers)
@@ -179,11 +186,6 @@ void convertNumber(const SRC& source, DST& target)
     }
     target = static_cast<DST>(source);
   }
-  // special case: bool accept truncation
-  else if constexpr(is_convertible_to_bool<SRC>() && is_same<DST, bool>())
-  {
-    target = static_cast<DST>(source);
-  }
   // casting to/from floating points might cause truncation.
   else if constexpr(std::is_floating_point<SRC>::value ||
                     std::is_floating_point<DST>::value)

include/behaviortree_cpp/utils/locked_reference.hpp

@@ -25,7 +25,7 @@ class LockedPtr
 
   ~LockedPtr()
   {
-    if(mutex_)
+    if(mutex_ != nullptr)
     {
       mutex_->unlock();
     }
@@ -34,16 +34,17 @@ class LockedPtr
   LockedPtr(LockedPtr const&) = delete;
   LockedPtr& operator=(LockedPtr const&) = delete;
 
-  LockedPtr(LockedPtr&& other)
+  LockedPtr(LockedPtr&& other) noexcept
   {
     std::swap(ref_, other.ref_);
     std::swap(mutex_, other.mutex_);
   }
 
-  LockedPtr& operator=(LockedPtr&& other)
+  LockedPtr& operator=(LockedPtr&& other) noexcept
   {
     std::swap(ref_, other.ref_);
     std::swap(mutex_, other.mutex_);
+    return *this;
   }
 
   operator bool() const
@@ -53,15 +54,15 @@ class LockedPtr
 
   void lock()
   {
-    if(mutex_)
+    if(mutex_ != nullptr)
     {
       mutex_->lock();
     }
   }
 
   void unlock()
   {
-    if(mutex_)
+    if(mutex_ != nullptr)
     {
       mutex_->unlock();
     }

include/behaviortree_cpp/utils/safe_any.hpp

@@ -70,7 +70,8 @@ class Any
   Any(const Any& other) : _any(other._any), _original_type(other._original_type)
   {}
 
-  Any(Any&& other) : _any(std::move(other._any)), _original_type(other._original_type)
+  Any(Any&& other) noexcept
+    : _any(std::move(other._any)), _original_type(other._original_type)
   {}
 
   explicit Any(const double& value) : _any(value), _original_type(typeid(double))
@@ -117,6 +118,8 @@ class Any
 
   Any& operator=(const Any& other);
 
+  Any& operator=(Any&& other) noexcept;
+
   [[nodiscard]] bool isNumber() const;
 
   [[nodiscard]] bool isIntegral() const;
@@ -305,7 +308,17 @@ inline bool isCastingSafe(const std::type_index& type, const T& val)
 
 inline Any& Any::operator=(const Any& other)
 {
-  this->_any = other._any;
+  if(this != &other)
+  {
+    this->_any = other._any;
+    this->_original_type = other._original_type;
+  }
+  return *this;
+}
+
+inline Any& Any::operator=(Any&& other) noexcept
+{
+  this->_any = std::move(other._any);
   this->_original_type = other._original_type;
   return *this;
 }

include/behaviortree_cpp/utils/simple_string.hpp

@@ -53,7 +53,6 @@ class SimpleString
     if(this != &other)
     {
       this->~SimpleString();
-      // Ensure clean state before swap
       _storage = {};
       std::swap(_storage, other._storage);
     }

include/behaviortree_cpp/utils/wildcards.hpp

@@ -1,5 +1,10 @@
 #pragma once
 
+#include <cstddef>
+#include <cstdint>
+#include <string_view>
+#include <vector>
+
 /**
  * @file wildcards.hpp
  * @brief Simple wildcard matching function supporting '*' and '?'.
@@ -35,7 +40,7 @@ inline bool wildcards_match(std::string_view str, std::string_view pattern)
       return cached == 1;
     }
 
-    bool result;
+    bool result = false;
     if(pattern[j] == '*')
     {
       result = match_ref(match_ref, i, j + 1);