Add Clang tidy and many fixes (#1047)

Author: facontidavide

.clang-tidy

@@ -0,0 +1,48 @@
+Checks: [
+  "-*",
+  "bugprone-*",
+  "cert-*",
+  "clang-analyzer-*",
+  "concurrency-*",
+  "cppcoreguidelines-*",
+  "misc-*",
+  "modernize-*",
+  "performance-*",
+  "portability-*",
+  "readability-*",
+  "-bugprone-easily-swappable-parameters",
+  "-bugprone-narrowing-conversions",
+  "-cert-err58-cpp",
+  "-cppcoreguidelines-avoid-c-arrays",
+  "-cppcoreguidelines-avoid-magic-numbers",
+  "-cppcoreguidelines-avoid-non-const-global-variables",
+  "-cppcoreguidelines-non-private-member-variables-in-classes",
+  "-cppcoreguidelines-pro-bounds-array-to-pointer-decay",
+  "-cppcoreguidelines-pro-bounds-constant-array-index",
+  "-cppcoreguidelines-pro-bounds-pointer-arithmetic",
+  "-cppcoreguidelines-pro-type-const-cast",
+  "-cppcoreguidelines-pro-type-union-access",
+  "-cppcoreguidelines-pro-type-vararg",
+  "-misc-no-recursion",
+  "-misc-non-private-member-variables-in-classes"
+]
+
+
+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

.github/dependabot.yml

@@ -0,0 +1,6 @@
+version: 2
+updates:
+  - package-ecosystem: "github-actions"
+    directory: "/"
+    schedule:
+      interval: "weekly"

.github/workflows/pre-commit.yaml

@@ -11,6 +11,29 @@ jobs:
   pre-commit:
     runs-on: ubuntu-latest
     steps:
-    - uses: actions/checkout@v3
-    - uses: actions/setup-python@v3
+    - uses: actions/checkout@v4
+    - uses: actions/setup-python@v5
     - uses: pre-commit/action@v3.0.1
+
+  clang-tidy:
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v4
+
+    - name: Install LLVM 21
+      run: |
+        wget https://apt.llvm.org/llvm.sh
+        chmod +x llvm.sh
+        sudo ./llvm.sh 21
+        sudo apt-get install -y clangd-21 clang-tidy-21
+
+    - name: Install dependencies
+      run: |
+        sudo apt-get update
+        sudo apt-get install -y libzmq3-dev libsqlite3-dev
+
+    - name: Configure CMake
+      run: cmake -B build -DBUILD_TESTING=OFF
+
+    - name: Run clang-tidy
+      run: ./run_clang_tidy.sh

.gitignore

@@ -5,8 +5,9 @@ site/*
 /.vscode/
 .vs/
 
-# clangd cache
+# clangd cache and config (generated by CMake)
 /.cache/*
+/.clangd
 CMakeSettings.json
 
 # OSX junk
@@ -18,3 +19,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

CMakeLists.txt

@@ -306,6 +306,21 @@ if(BTCPP_EXAMPLES)
     add_subdirectory(examples)
 endif()
 
+######################################################
+# Generate .clangd configuration file for standalone header checking
+file(WRITE ${CMAKE_SOURCE_DIR}/.clangd
+"CompileFlags:
+  Add:
+    - -xc++
+    - -std=c++17
+    - -I${CMAKE_SOURCE_DIR}/include
+    - -I${CMAKE_SOURCE_DIR}/3rdparty
+    - -I${CMAKE_SOURCE_DIR}/3rdparty/minitrace
+    - -I${CMAKE_SOURCE_DIR}/3rdparty/tinyxml2
+    - -I${CMAKE_SOURCE_DIR}/3rdparty/minicoro
+    - -I${CMAKE_SOURCE_DIR}/3rdparty/lexy/include
+")
+
 ######################################################
 # INSTALL
 

include/behaviortree_cpp/action_node.h

@@ -38,6 +38,11 @@ class ActionNodeBase : public LeafNode
   ActionNodeBase(const std::string& name, const NodeConfig& config);
   ~ActionNodeBase() override = default;
 
+  ActionNodeBase(const ActionNodeBase&) = delete;
+  ActionNodeBase& operator=(const ActionNodeBase&) = delete;
+  ActionNodeBase(ActionNodeBase&&) = delete;
+  ActionNodeBase& operator=(ActionNodeBase&&) = delete;
+
   virtual NodeType type() const override final
   {
     return NodeType::ACTION;
@@ -55,6 +60,11 @@ class SyncActionNode : public ActionNodeBase
   SyncActionNode(const std::string& name, const NodeConfig& config);
   ~SyncActionNode() override = default;
 
+  SyncActionNode(const SyncActionNode&) = delete;
+  SyncActionNode& operator=(const SyncActionNode&) = delete;
+  SyncActionNode(SyncActionNode&&) = delete;
+  SyncActionNode& operator=(SyncActionNode&&) = delete;
+
   /// throws if the derived class return RUNNING.
   virtual NodeStatus executeTick() override;
 
@@ -87,6 +97,11 @@ class SimpleActionNode : public SyncActionNode
 
   ~SimpleActionNode() override = default;
 
+  SimpleActionNode(const SimpleActionNode&) = delete;
+  SimpleActionNode& operator=(const SimpleActionNode&) = delete;
+  SimpleActionNode(SimpleActionNode&&) = delete;
+  SimpleActionNode& operator=(SimpleActionNode&&) = delete;
+
 protected:
   virtual NodeStatus tick() override final;
 
@@ -197,7 +212,12 @@ class CoroActionNode : public ActionNodeBase
 {
 public:
   CoroActionNode(const std::string& name, const NodeConfig& config);
-  virtual ~CoroActionNode() override;
+  ~CoroActionNode() override;
+
+  CoroActionNode(const CoroActionNode&) = delete;
+  CoroActionNode& operator=(const CoroActionNode&) = delete;
+  CoroActionNode(CoroActionNode&&) = delete;
+  CoroActionNode& operator=(CoroActionNode&&) = delete;
 
   /// Use this method to return RUNNING and temporary "pause" the Action.
   void setStatusRunningAndYield();

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

@@ -22,6 +22,11 @@ class SleepNode : public StatefulActionNode
     halt();
   }
 
+  SleepNode(const SleepNode&) = delete;
+  SleepNode& operator=(const SleepNode&) = delete;
+  SleepNode(SleepNode&&) = delete;
+  SleepNode& operator=(SleepNode&&) = delete;
+
   NodeStatus onStart() override;
 
   NodeStatus onRunning() override;
@@ -35,7 +40,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/actions/updated_action.h

@@ -30,6 +30,11 @@ class EntryUpdatedAction : public SyncActionNode
 
   ~EntryUpdatedAction() override = default;
 
+  EntryUpdatedAction(const EntryUpdatedAction&) = delete;
+  EntryUpdatedAction& operator=(const EntryUpdatedAction&) = delete;
+  EntryUpdatedAction(EntryUpdatedAction&&) = delete;
+  EntryUpdatedAction& operator=(EntryUpdatedAction&&) = delete;
+
   static PortsList providedPorts()
   {
     return { InputPort<BT::Any>("entry", "Entry to check") };