Add Clang tidy and many fixes

[facontidavide]

Summary by CodeRabbit

• Bug Fixes

  • Safer parsing, stricter null/mutex checks and more robust timer handling to reduce runtime issues.

• Refactor

  • Widespread const-correctness, explicit initializers and simplified control flow for improved stability and readability.

• Chores

  • Added static-analysis config, runner script and CI clang-tidy job; updated ignore rules; removed the example/demo.

• Tests

  • New comprehensive unit tests for string utilities.

• Breaking Changes

  • Many public types made non-copyable/non-movable; two tree-building helper APIs removed — update callers accordingly.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Adds clang‑tidy tooling and CI; introduces many non‑copyable/non‑movable public types; broad const/noexcept/default‑init fixes; reworks TimerQueue and decorator timer logic; improves SafeAny/SimpleString and conversion checks; removes example; adds SimpleString tests; assorted safety and serialization cleanups.

Changes

Cohort / File(s)	Summary
Static analysis & CI \.clang-tidy, run_clang_tidy.sh, \.pre-commit-config.yaml, \.gitignore, CMakeLists.txt, .github/workflows/pre-commit.yaml	Add clang‑tidy config, runner script, CI job, CMake rule to emit .clangd, example pre‑commit hook (commented), and gitignore entries.
Tests tests/CMakeLists.txt, tests/gtest_simple_string.cpp	Add comprehensive SimpleString unit tests and include them in test build.
String / Any / Conversions include/behaviortree_cpp/utils/simple_string.hpp, include/behaviortree_cpp/utils/safe_any.hpp, include/behaviortree_cpp/utils/convert_impl.hpp, include/behaviortree_cpp/utils/wildcards.hpp, include/behaviortree_cpp/utils/locked_reference.hpp	SOO default ctor and inits; noexcept move ops; self‑assignment guards; improved Any move/assign semantics; tightened convertNumber with explicit float/int→bool handling and bounds checks; initialize previously uninitialized locals.
Timer & scheduling core include/behaviortree_cpp/utils/timer_queue.h, src/decorators/delay_node.cpp, src/decorators/timeout_node.cpp	Template rename to ClockT/DurationT, uniform time_point types, delete TimerQueue move ops; Delay/Timeout nodes refactored to timer_id model, const locks, wake/cancel flow and cleaner locking/unlock sequences.
Blackboard & JSON export include/behaviortree_cpp/blackboard.h, src/blackboard.cpp, src/json_export.cpp, include/behaviortree_cpp/json_export.h	Make Blackboard/Entry non‑copyable/non‑movable; JsonExporter made non‑instantiable and non‑copyable; small JSON exporter cleanups and const usage.
Public API: Node / Control / Decorator / Logger headers include/behaviortree_cpp/*.h (e.g., action_node.h, control_node.h, decorators/*, controls/*, loggers/*, blackboard.h, json_export.h)	Widespread deletion of copy/move special members across many node/logger/decorator/control types; destructor declaration style adjustments; some in‑class default initializers added.
Core implementations & safety src/*.cpp, src/**/**/*.cpp (e.g., action_node.cpp, tree_node.cpp, bt_factory.cpp, xml_parsing.cpp, shared_library*.cpp, script_parser.cpp)	Const‑qualify locals and lock objects, explicit nullptr comparisons, noexcept move declarations, anonymous namespaces for internals, make_unique usage, safer parsing and factory pointer usage, removal of example build functions, and other initialization/safety fixes.
Loggers & publishers src/loggers/*, src/loggers/groot2_publisher.cpp, include/behaviortree_cpp/loggers/*	Move helpers to anonymous namespaces, replace C buffers with std::array, const locks, defensive null checks, try/catch in destructors, finalize sqlite statements, and delete logger copy/move ops.
Controls & decorators (impls) src/controls/*, src/decorators/*	Pointer/type correctness, const locals, loop/formatting fixes (ManualSelector), removal of some static declarations, and refactors to align with header API changes and timer model.
Examples & housekeeping src/example.cpp, various headers, .gitignore, copyright headers	Remove example program; update .gitignore and many header years; minor unused‑local removals and formatting tweaks.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Poem

🐰
I nudged the headers, tightened every lock,
made moves noexcept and trimmed a linted flock.
Small strings hop in tests, timers wake in line,
tidy flags are flying — the tree feels fine.
A carrot for the repo, neat, robust, and kind. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description only contains the template reminder about clang-format and pre-commit without substantive details about the changes, objectives, or scope of the work performed.	Replace the template comment with a clear description of the clang-tidy integration, the categories of fixes applied (const-correctness, null checks, deleted special members), and testing guidance.
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Add Clang tidy and many fixes' accurately reflects the primary change: introducing clang-tidy tooling and making multiple code improvements to align with static analysis standards.

---

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0f37983 and 0af35ed.

📒 Files selected for processing (2)

• src/bt_factory.cpp
• src/tree_node.cpp

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/behavior_tree.cpp (1)

35-38: Inconsistent nullptr guard between const and non-const applyRecursiveVisitor.

The non-const version (lines 59-62) now guards against decorator->child() != nullptr before recursing, but the const version (line 37) does not have this guard. If a decorator can have a null child, both versions should handle it consistently.

🔎 Proposed fix for const version

   else if(auto decorator = dynamic_cast<const BT::DecoratorNode*>(node))
   {
-    applyRecursiveVisitor(decorator->child(), visitor);
+    if(decorator->child() != nullptr)
+    {
+      applyRecursiveVisitor(decorator->child(), visitor);
+    }
   }

Also applies to: 57-63

src/decorators/delay_node.cpp (1)

42-49: The config-based constructor is missing initialization of read_parameter_from_ports_.

The review comment correctly identifies a critical issue: the DelayNode config-based constructor at line 15 initializes msec_ to 0 but never sets read_parameter_from_ports_ to true. This prevents the port-based configuration from working, even though the header declares InputPort<unsigned>("delay_msec") and the documentation promises the delay can be passed via port. Other similar nodes (timeout_node, retry_node, repeat_node, parallel_node) all explicitly set read_parameter_from_ports_(true) in their config-based constructors. The config-based constructor should do the same.

🧹 Nitpick comments (4)

src/controls/reactive_sequence.cpp (1)

58-58: LGTM! The explicit cast improves type safety.

The change to use static_cast<int>(index) makes the size_t-to-int conversion explicit, which is a good practice for type safety and aligns with clang-tidy recommendations.

Optional: Consider consistent casting style throughout the method.

For consistency, line 56 could also use static_cast<int>(index) instead of int(index):

🔎 Optional consistency improvement

-        running_child_ = int(index);
+        running_child_ = static_cast<int>(index);

include/behaviortree_cpp/utils/convert_impl.hpp (1)

157-171: Inconsistent behavior between floating-point and integer to bool conversions.

Floating-point values allow C-style conversion (any non-zero becomes true), but integer values only allow 0 or 1. This means 2.0 converts to true, while integer 2 throws an exception.

If the intent is strict conversion for safety, consider also validating floating-point values:

if constexpr (std::is_floating_point<SRC>::value && is_same<DST, bool>())
{
  if(source != 0.0 && source != 1.0)
  {
    throw std::runtime_error("Implicit casting to bool is not allowed");
  }
  target = static_cast<DST>(source);
}

Otherwise, if C-style permissiveness is desired, the integer path could also allow any non-zero value.

src/basic_types.cpp (1)

200-206: Good const-correctness improvements.

The refactoring correctly saves and restores the locale. Note that setlocale is not thread-safe (it modifies global state), but this appears to be pre-existing behavior. For thread-safe parsing, consider std::strtod with a thread-local or std::from_chars (C++17 for floating-point, though support varies).

src/loggers/groot2_publisher.cpp (1)

103-103: Consider fixing the typo in the variable name.

The variable name recording_fist_time appears to be a typo and should likely be recording_first_time. This name is used throughout the file (lines 221, 440).

🔎 Proposed fix

-  std::chrono::microseconds recording_fist_time{};
+  std::chrono::microseconds recording_first_time{};

Then update references on lines 221 and 440 accordingly.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4afefac and f1906d8.

📒 Files selected for processing (47)

• .clang-tidy
• .gitignore
• .pre-commit-config.yaml
• include/behaviortree_cpp/actions/pop_from_queue.hpp
• include/behaviortree_cpp/actions/sleep_node.h
• include/behaviortree_cpp/utils/convert_impl.hpp
• include/behaviortree_cpp/utils/locked_reference.hpp
• include/behaviortree_cpp/utils/safe_any.hpp
• include/behaviortree_cpp/utils/simple_string.hpp
• include/behaviortree_cpp/utils/wildcards.hpp
• run_clang_tidy.sh
• src/action_node.cpp
• src/actions/sleep_node.cpp
• src/actions/updated_action.cpp
• src/basic_types.cpp
• src/behavior_tree.cpp
• src/blackboard.cpp
• src/bt_factory.cpp
• src/control_node.cpp
• src/controls/fallback_node.cpp
• src/controls/if_then_else_node.cpp
• src/controls/manual_node.cpp
• src/controls/parallel_all_node.cpp
• src/controls/reactive_fallback.cpp
• src/controls/reactive_sequence.cpp
• src/controls/sequence_node.cpp
• src/controls/switch_node.cpp
• src/controls/while_do_else_node.cpp
• src/decorator_node.cpp
• src/decorators/delay_node.cpp
• src/decorators/repeat_node.cpp
• src/decorators/retry_node.cpp
• src/decorators/subtree_node.cpp
• src/decorators/timeout_node.cpp
• src/decorators/updated_decorator.cpp
• src/example.cpp
• src/json_export.cpp
• src/loggers/bt_cout_logger.cpp
• src/loggers/bt_file_logger_v2.cpp
• src/loggers/bt_minitrace_logger.cpp
• src/loggers/bt_sqlite_logger.cpp
• src/loggers/groot2_publisher.cpp
• src/script_parser.cpp
• src/shared_library.cpp
• src/shared_library_UNIX.cpp
• src/tree_node.cpp
• src/xml_parsing.cpp

💤 Files with no reviewable changes (1)

• src/example.cpp

🧰 Additional context used

🧬 Code graph analysis (19)

src/decorators/subtree_node.cpp (2)

src/tree_node.cpp (2)

• status (276-280)
• status (276-276)

tests/navigation_test.cpp (1)

• status (126-146)

src/loggers/bt_cout_logger.cpp (1)

include/behaviortree_cpp/loggers/bt_cout_logger.h (1)

• StdCoutLogger (14-25)

src/loggers/bt_file_logger_v2.cpp (2)

src/tree_node.cpp (2)

• status (276-280)
• status (276-276)

src/shared_library_UNIX.cpp (3)

• lock (13-13)
• lock (32-32)
• lock (48-48)

src/controls/fallback_node.cpp (1)

src/tree_node.cpp (2)

• setRegistrationID (433-436)
• setRegistrationID (433-433)

include/behaviortree_cpp/utils/locked_reference.hpp (3)

include/behaviortree_cpp/utils/simple_string.hpp (8)

• other (36-44)
• other (36-36)
• other (51-59)
• other (51-51)
• other (111-115)
• other (111-111)
• other (117-121)
• other (117-117)

include/behaviortree_cpp/utils/safe_any.hpp (6)

• other (119-119)
• other (121-121)
• other (309-317)
• other (309-309)
• other (319-324)
• other (319-319)

src/tree_node.cpp (2)

• other (60-64)
• other (60-60)

src/controls/sequence_node.cpp (1)

src/tree_node.cpp (2)

• setRegistrationID (433-436)
• setRegistrationID (433-433)

src/decorators/updated_decorator.cpp (5)

src/loggers/groot2_publisher.cpp (2)

• lk (118-118)
• lk (198-198)

src/loggers/bt_sqlite_logger.cpp (2)

• lk (189-189)
• lk (208-208)

src/actions/sleep_node.cpp (1)

• lk (28-28)

src/decorators/delay_node.cpp (2)

• lk (43-43)
• lk (52-52)

src/decorators/timeout_node.cpp (2)

• lk (44-44)
• lk (55-55)

src/control_node.cpp (1)

src/decorator_node.cpp (4)

• child (38-41)
• child (38-38)
• child (43-46)
• child (43-43)

src/bt_factory.cpp (1)

src/behavior_tree.cpp (4)

• applyRecursiveVisitor (18-39)
• applyRecursiveVisitor (18-19)
• applyRecursiveVisitor (41-64)
• applyRecursiveVisitor (41-41)

src/action_node.cpp (3)

src/tree_node.cpp (10)

• name (293-296)
• name (293-293)
• config (345-348)
• config (345-345)
• config (350-353)
• config (350-350)
• setStatus (151-172)
• setStatus (151-151)
• emitWakeUpSignal (420-426)
• emitWakeUpSignal (420-420)

3rdparty/minicoro/minicoro.h (2)

• mco_create (1684-1711)
• mco_destroy (1713-1729)

src/bt_factory.cpp (2)

• emitWakeUpSignal (525-528)
• emitWakeUpSignal (525-525)

src/controls/while_do_else_node.cpp (3)

src/control_node.cpp (2)

• resetChildren (38-48)
• resetChildren (38-38)

src/tree_node.cpp (2)

• status (276-280)
• status (276-276)

tests/navigation_test.cpp (1)

• status (126-146)

src/actions/updated_action.cpp (5)

src/loggers/groot2_publisher.cpp (2)

• lk (118-118)
• lk (198-198)

src/loggers/bt_sqlite_logger.cpp (2)

• lk (189-189)
• lk (208-208)

src/actions/sleep_node.cpp (1)

• lk (28-28)

src/decorators/delay_node.cpp (2)

• lk (43-43)
• lk (52-52)

src/decorators/timeout_node.cpp (2)

• lk (44-44)
• lk (55-55)

src/decorators/delay_node.cpp (1)

include/behaviortree_cpp/decorators/delay_node.h (1)

• DelayNode (34-66)

src/controls/if_then_else_node.cpp (2)

src/tree_node.cpp (2)

• status (276-280)
• status (276-276)

src/control_node.cpp (2)

• resetChildren (38-48)
• resetChildren (38-38)

src/shared_library.cpp (1)

include/behaviortree_cpp/exceptions.h (1)

• RuntimeError (58-67)

src/decorators/timeout_node.cpp (2)

src/decorator_node.cpp (6)

• child (38-41)
• child (38-38)
• child (43-46)
• child (43-43)
• resetChild (53-64)
• resetChild (53-53)

include/behaviortree_cpp/basic_types.h (1)

• isStatusCompleted (47-50)

src/tree_node.cpp (1)

include/behaviortree_cpp/tree_node.h (1)

• TreeNode (131-295)

src/shared_library_UNIX.cpp (3)

src/shared_library.cpp (1)

• SharedLibrary (4-7)

include/behaviortree_cpp/utils/shared_library.h (1)

• SharedLibrary (47-139)

include/behaviortree_cpp/exceptions.h (1)

• RuntimeError (58-67)

include/behaviortree_cpp/utils/safe_any.hpp (1)

include/behaviortree_cpp/utils/simple_string.hpp (16)

• other (36-44)
• other (36-36)
• other (51-59)
• other (51-51)
• other (111-115)
• other (111-111)
• other (117-121)
• other (117-117)
• other (123-126)
• other (123-123)
• other (128-131)
• other (128-128)
• other (133-142)
• other (133-133)
• other (144-153)
• other (144-144)

🪛 Cppcheck (2.19.0)

src/xml_parsing.cpp

[error] 376-376: Found an exit path from function with non-void return type that has missing return statement

(missingReturn)

🪛 GitHub Actions: cmake Ubuntu

src/bt_factory.cpp

[error] 109-109: Error: 'const std::set<std::__cxx11::basic_string >' has no member named 'contains'. Use 'find' or 'count' instead.

🪛 Shellcheck (0.11.0)

run_clang_tidy.sh

[warning] 62-62: Quote this to prevent word splitting.

(SC2046)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)

• GitHub Check: build (windows-latest)
• GitHub Check: pixi_conda_build (windows-latest)
• GitHub Check: pixi_conda_build (ubuntu-latest)
• GitHub Check: industrial_ci (humble, main)
• GitHub Check: industrial_ci (jazzy, main)
• GitHub Check: industrial_ci (rolling, main)
• GitHub Check: build (ubuntu-22.04, tsan)
• GitHub Check: build (ubuntu-22.04, asan_ubsan)

🔇 Additional comments (117)

src/json_export.cpp (1)

91-91: Good const-correctness improvement.

Marking first_element as const is appropriate since it's only used for type inspection and is never modified. This aligns with clang-tidy recommendations and modern C++ best practices.

src/script_parser.cpp (5)

26-26: LGTM: const-correctness improvement.

Marking exprs as const is appropriate since the vector is not modified after initialization. This is a typical clang-tidy recommendation for improved immutability.

---

35-35: Unsigned literal aligns with size_t comparison.

Using 0U makes the loop index type consistent with exprs.size(), which returns size_t (unsigned). The loop logic correctly handles all cases given the empty check at lines 27-30.

---

52-52: LGTM: reduced nesting by removing redundant else.

Since all paths within the if block return, the else is unnecessary. This change reduces indentation and aligns with clang-tidy's readability recommendations.

---

62-63: LGTM: simplified control flow.

Removing the else block and adding the forwarding comment clarifies that this path only executes when executor is false. The change reduces nesting while preserving the same behavior.

---

78-78: LGTM: const-correctness improvement.

Same as line 26—marking the parsed expressions vector as const is appropriate since it's not mutated after initialization.

src/controls/reactive_fallback.cpp (1)

58-58: LGTM: Modernized cast to static_cast.

The change from C-style cast to static_cast<int> correctly modernizes the code and aligns with clang-tidy best practices for explicit type conversions.

include/behaviortree_cpp/utils/wildcards.hpp (2)

3-6: LGTM: Explicit includes improve maintainability.

Adding explicit includes for <cstddef>, <cstdint>, <string_view>, and <vector> ensures that the file declares all dependencies rather than relying on transitive includes. This prevents potential breakage if included headers change their own dependencies in the future.

---

43-43: LGTM: Initialization ensures defined behavior.

Initializing result to false at declaration is defensive programming that prevents potential static analyzer warnings about uninitialized variables. While all current code paths assign to result before use, this initialization guards against future logic changes and aligns with the PR's clang-tidy objectives.

include/behaviortree_cpp/actions/pop_from_queue.hpp (2)

74-79: LGTM! Clean refactoring that improves readability.

The removal of the redundant else block after the early return (line 72) follows clang-tidy's readability-else-after-return guideline. The logic remains functionally equivalent—lines 74-77 execute only when the queue is non-empty, and line 79 correctly handles the case where queue retrieval fails or the pointer is null.

---

122-123: LGTM! Consistent refactoring improves code clarity.

This change mirrors the PopFromQueue refactoring by removing the unnecessary else block after the early return at line 120. The simplified control flow is clearer and follows the same clang-tidy guideline.

src/basic_types.cpp (6)

9-9: LGTM!

The <tuple> include is needed for std::ignore usage below.

---

121-122: LGTM!

Using std::ignore to explicitly discard the unused ptr is a clean way to silence warnings.

---

143-156: LGTM!

The anonymous namespace properly encapsulates this internal helper. The bounds checking via int64_t is safe since all target types (8/16/32-bit signed and unsigned) fit within int64_t range.

---

447-447: LGTM!

Casting to unsigned char before isalpha avoids undefined behavior when char is signed and has a negative value.

---

475-475: LGTM!

Making the parsed JSON object const correctly reflects that it's not modified after parsing.

---

494-508: LGTM!

The explicit size check and manual comparison is correct. If targeting C++20 or later, std::string_view::starts_with() could simplify this, but the current implementation is compatible with C++17.

src/loggers/groot2_publisher.cpp (11)

4-4: LGTM! Good use of anonymous namespace and necessary include.

The <tuple> include supports std::ignore usage later in the file. Wrapping CreateRandomUUID in an anonymous namespace properly limits its linkage to this translation unit, which is appropriate for internal helper functions.

Also applies to: 33-57

---

37-40: LGTM! Safe zero-initialization.

The explicit zero-initialization with {} ensures deterministic initial state for the array.

---

44-49: LGTM! Explicit casts improve type safety.

The static_cast<char> additions make narrowing conversions explicit and prevent potential issues with signed char types in bitwise operations.

---

66-66: LGTM! Improved const-correctness.

Marking these timeout values as const correctly documents their immutability and prevents accidental modification.

Also applies to: 70-70

---

118-118: LGTM! Improved const-correctness for lock guards.

Marking these std::unique_lock instances as const correctly documents that they are not reassigned or moved after construction. This is appropriate for scoped RAII lock guards.

Also applies to: 198-198, 207-207, 289-289, 417-417, 446-446, 460-460, 489-489, 591-591, 598-598, 659-659, 707-707

---

212-212: LGTM! Type-safe conversions and initialization.

The explicit static_cast chain ensures type safety through arithmetic operations, and zero-initialization of the Transition struct provides deterministic initial state.

Also applies to: 218-218

---

320-320: LGTM! Consistent const-correctness.

These variables are correctly marked const as they are not modified after initialization.

Also applies to: 364-367, 405-405

---

419-421: LGTM! Clear marking of intentionally unused variables.

Using std::ignore to explicitly mark unused structured binding elements is a good practice that documents intent and silences compiler warnings.

Also applies to: 490-492, 689-691

---

513-513: LGTM! Appropriate const qualification.

The const qualification correctly documents that this captured state is not modified.

---

554-554: LGTM! Proper const qualification of pointers.

These TreeNode::Ptr instances are correctly marked const as they are not reassigned after initialization. The pointed-to objects remain mutable as intended.

Also applies to: 613-613, 646-646

---

568-568: LGTM! Const-correct object.

The const qualification is appropriate as this request header is not modified after construction.

.gitignore (1)

21-23: LGTM! Appropriate gitignore entries for clang-tidy tooling.

The added entries correctly exclude clang-tidy related artifacts from version control.

include/behaviortree_cpp/actions/sleep_node.h (1)

38-38: LGTM! Good defensive programming practice.

Initializing timer_id_ to 0 prevents undefined behavior and is a best practice for member variables.

src/controls/sequence_node.cpp (1)

22-29: LGTM! Improved consistency with explicit braces.

Adding braces to both branches follows common coding standards and reduces the risk of future errors when modifying these blocks.

src/controls/switch_node.cpp (1)

82-82: LGTM! Clean simplification of conditional logic.

Converting the multi-line if-statement to a single return expression improves readability while preserving the exact same behavior.

.pre-commit-config.yaml (1)

46-53: LGTM! Useful documentation for optional clang-tidy integration.

The commented-out hook provides a clear example for contributors who want to enable clang-tidy in their pre-commit workflow.

src/controls/fallback_node.cpp (1)

21-28: LGTM! Consistent with similar refactoring in sequence_node.cpp.

Adding explicit braces improves code consistency across control nodes and aligns with common coding standards.

src/shared_library.cpp (1)

12-16: LGTM! Improved clarity with explicit nullptr comparison.

The explicit != nullptr check is more readable and aligns with modern C++ best practices. The control flow remains identical.

src/controls/parallel_all_node.cpp (1)

50-50: LGTM! Modern and more expressive membership check.

Using .contains() is more idiomatic than .count() != 0 for checking set membership and clearly expresses the intent.

src/decorators/repeat_node.cpp (1)

50-51: LGTM: Const-correctness improvement.

Making these local status variables const is appropriate since they're not modified after initialization. This aligns with modern C++ best practices and clang-tidy recommendations.

src/controls/while_do_else_node.cpp (2)

39-39: LGTM: Const-correctness improvement.

Making condition_status const is appropriate since it's not modified after initialization.

---

73-74: LGTM: Control flow simplification.

The removal of the else block is correct. After the early return for RUNNING at line 71, the code always resets children and returns status. This simplifies the logic without changing behavior.

src/decorators/subtree_node.cpp (1)

22-22: LGTM: Const-correctness improvement.

Making prev_status const is appropriate since it's not modified after initialization.

src/decorators/updated_decorator.cpp (1)

53-53: LGTM: Const-qualified lock for read-only critical section.

Making the std::unique_lock const is appropriate here since the lock object itself doesn't need to be modified (no explicit unlock or lock operations). The RAII semantics still work correctly.

src/actions/sleep_node.cpp (1)

28-28: LGTM: Const-qualified lock in lambda.

Making the lock const within the timer callback is appropriate since the lock object doesn't need to be modified. The RAII semantics remain intact.

src/actions/updated_action.cpp (1)

43-43: LGTM: Const-qualified lock for read-only critical section.

Consistent with the pattern used throughout this PR. The lock protects a read-only access to entry->sequence_id, so making it const is appropriate.

src/loggers/bt_minitrace_logger.cpp (1)

24-44: LGTM: Internal linkage for helper function.

Moving toConstStr into an anonymous namespace gives it internal linkage, which is best practice for helper functions that don't need to be exported. This prevents potential ODR violations.

src/control_node.cpp (2)

40-40: LGTM: Explicit pointer type.

Using auto* instead of auto makes the pointer type explicit and improves code clarity. This aligns with modern C++ best practices and clang-tidy recommendations.

---

57-57: LGTM: Explicit pointer type.

Making the pointer type explicit with auto* improves code clarity and consistency with the change at line 40.

src/decorators/retry_node.cpp (1)

55-56: LGTM!

Adding const to prev_status and child_status is appropriate since these values are captured once per loop iteration and never modified. This aligns with the broader const-correctness improvements in this PR.

src/decorator_node.cpp (3)

24-24: LGTM!

Explicit != nullptr comparison is clearer than implicit truthiness check, aligning with clang-tidy best practices.

---

55-55: LGTM!

Consistent use of explicit == nullptr for the early return guard.

---

79-80: LGTM!

Both status and child_status are only read after assignment, making const appropriate here.

src/loggers/bt_cout_logger.cpp (2)

8-8: LGTM!

Defaulted destructor is appropriate here since no custom cleanup logic is required.

---

18-18: LGTM!

Adding const to since_epoch correctly reflects its immutable nature.

src/controls/if_then_else_node.cpp (3)

42-42: LGTM!

Adding const to condition_status is correct since the value is evaluated once and not modified.

---

48-61: LGTM!

The restructured conditional logic is clearer. For FAILURE:

• If 3 children exist, proceed to else branch (child_idx_ = 2)
• Otherwise, return FAILURE immediately

This correctly handles both 2-child and 3-child configurations.

---

67-74: LGTM!

The simplified control flow correctly resets children and returns the status for any non-RUNNING outcome, eliminating the redundant else branch.

src/loggers/bt_file_logger_v2.cpp (4)

8-14: LGTM!

Wrapping ToUsec in an anonymous namespace correctly restricts its linkage to this translation unit.

---

57-59: LGTM!

Using std::array<char, 8> with value initialization and data() accessor is safer and more idiomatic than raw C-style buffers.

---

85-90: LGTM!

Value-initializing Transition with {} and using const std::scoped_lock aligns with the project's const-correctness patterns.

---

120-120: LGTM!

Using std::array<char, 9>{} ensures zero-initialization of the write buffer before populating it with memcpy.

src/decorators/timeout_node.cpp (2)

44-44: LGTM!

Adding const to the unique_lock in the timer callback correctly reflects that the lock variable itself isn't reassigned.

---

62-71: Pattern is safe and no race condition exists.

The unlock/relock window around timer_.cancel() is intentionally designed to prevent deadlock while maintaining correctness. During the unlock window (lines 66-68), the timer callback cannot corrupt state because:

• child_status is already completed when reaching line 63, meaning child()->status() is not RUNNING
• The timer callback's condition at line 45 checks exactly this (if(child()->status() == RUNNING))
• Since the child is not running, the callback will not execute any state-modifying operations
• child_halted_ is std::atomic_bool, ensuring thread-safe updates regardless of timing
• Mutex re-acquisition at line 68 synchronizes before calling resetChild()

The pattern correctly avoids deadlock while ensuring no data races occur.

src/loggers/bt_sqlite_logger.cpp (4)

21-25: LGTM!

Good defensive null-check for err_msg before using it. sqlite3_exec can leave err_msg as nullptr in certain error conditions.

---

133-145: LGTM!

Wrapping destructor operations in try/catch is essential to prevent exceptions from escaping and causing std::terminate. The sqlite3_close(db_) correctly remains outside the try block to ensure the database handle is always closed.

---

157-157: LGTM!

Adding const to tm_usec correctly reflects its immutable use after assignment.

---

189-189: LGTM!

Using const std::scoped_lock aligns with the project's const-correctness patterns for lock objects.

src/action_node.cpp (4)

75-81: LGTM!

Wrapping CoroEntry in an anonymous namespace correctly restricts its linkage to this translation unit, preventing symbol conflicts.

---

89-96: LGTM!

Essential try/catch in the destructor prevents exceptions from destroyCoroutine() escaping and causing std::terminate.

---

148-156: LGTM!

The explicit nullptr check before calling mco_destroy adds clarity, even though mco_destroy handles null pointers internally (returns MCO_INVALID_COROUTINE). The const on res correctly reflects its immutable use.

---

224-232: LGTM!

Adding const to the lock_type variables correctly reflects that the lock objects themselves aren't reassigned after construction.

include/behaviortree_cpp/utils/locked_reference.hpp (2)

37-48: Verify move assignment self-assignment safety.

The move assignment operator uses std::swap without a self-assignment guard. While swapping with oneself is typically safe and results in a no-op, adding an explicit check (if (this != &other)) would make the intent clearer and align with best practices seen in other files in this PR (e.g., safe_any.hpp, blackboard.cpp).

However, this may be acceptable as-is since std::swap(x, x) is well-defined. Consider whether consistency with the rest of the codebase is more important than the minor performance cost of the check.

---

28-31: LGTM! Explicit nullptr checks improve clarity.

The changes from implicit truthiness checks to explicit != nullptr comparisons make the code more readable and align with modern C++ best practices.

Also applies to: 57-60, 65-68

src/shared_library_UNIX.cpp (3)

9-9: LGTM! Defaulted constructor is appropriate.

Since _handle has an in-class initializer (= nullptr in the header), using = default is the idiomatic choice and eliminates the explicit initialization in the constructor body.

---

13-13: Excellent const-correctness improvement.

Marking locks as const prevents accidental modifications to the lock object itself and makes the code safer. This aligns with the broader const-correctness improvements across the PR.

Also applies to: 32-32, 48-48

---

15-15: LGTM! Explicit nullptr comparisons improve readability.

The changes from implicit boolean conversion to explicit != nullptr and == nullptr comparisons make the intent clearer and align with modern C++ best practices.

Also applies to: 21-25, 34-34, 51-51

src/controls/manual_node.cpp (4)

76-76: LGTM! Const qualifier prevents accidental modification.

Using const for the result variable is good practice and prevents accidental reassignment.

---

86-86: Verify control flow behavior with separate if statements.

The code at lines 109 and 114 changed from else if to separate if statements. This means if ch matches multiple conditions (which shouldn't happen with keyboard input), multiple branches could execute. While this is likely safe since the loop breaks in each branch, the original else if chain was more explicit about mutual exclusivity.

The current implementation is functionally equivalent for valid keyboard input but slightly less clear about intent. Consider whether the original else if chain was preferable for documenting mutual exclusivity.

Also applies to: 101-120

---

102-102: LGTM! while(true) is more idiomatic than while(1).

This change improves readability and is a standard modernization.

Also applies to: 169-169

---

149-149: Verify format specifier compatibility for size_t.

Line 159 uses %2zu for size_t, which is correct. Line 149 casts children_count to int for newwin() height parameter. Ensure that children_count cannot exceed INT_MAX in practice, as the cast could overflow for very large values (though this is extremely unlikely for a behavior tree).

Also applies to: 159-159

include/behaviortree_cpp/utils/safe_any.hpp (3)

73-75: LGTM! Move constructor correctly marked noexcept.

The move constructor properly initializes members and is marked noexcept, which is appropriate since moving the internal linb::any and copying the type index should not throw.

---

309-317: LGTM! Self-assignment guard protects copy assignment.

The copy assignment operator includes a proper self-assignment check (if(this != &other)), which prevents unnecessary work and potential issues.

---

319-324: Add self-assignment guard to move assignment operator.

The move assignment operator lacks a self-assignment check. While self-move is rare and often considered undefined behavior, adding a guard (if(this != &other)) would:

1. Make the code consistent with the copy assignment operator
2. Prevent potential issues with self-move scenarios
3. Align with best practices seen in SimpleString (lines 50-58 in relevant_code_snippets)

🔎 Proposed fix

 inline Any& Any::operator=(Any&& other) noexcept
 {
+  if(this != &other)
+  {
     this->_any = std::move(other._any);
     this->_original_type = other._original_type;
+  }
   return *this;
 }

Likely an incorrect or invalid review comment.

src/xml_parsing.cpp (8)

89-94: LGTM! Anonymous namespace appropriately scopes helper.

Moving the StrEqual lambda into an anonymous namespace is good practice and prevents potential naming conflicts.

---

121-121: Question: Why change factory from reference to pointer?

The factory member was changed from a reference to a pointer initialized to nullptr. This introduces the possibility of null dereference and requires checking throughout. References are preferred when an object must always be valid.

Was this change necessary to support some use case, or could the reference have been retained?

Also applies to: 129-129

---

151-152: LGTM! Move constructor properly marked noexcept.

The move constructor for XMLParser correctly specifies noexcept, which is appropriate when moving a unique_ptr.

---

165-165: Excellent use of std::make_unique.

Replacing raw new with std::make_unique is safer and exception-safe. This is a best practice modernization.

Also applies to: 177-177, 314-314

---

244-246: LGTM! Proper use of std::ignore for unused variables.

Using std::ignore to explicitly mark unused structured binding elements or variables is good practice and silences compiler warnings appropriately.

Also applies to: 370-371, 378-379, 777-777

---

882-882: LGTM! Pointer-based dynamic_cast is safer.

Using dynamic_cast to pointers instead of references is safer because it returns nullptr on failure rather than throwing an exception. This aligns with the explicit nullptr checking pattern throughout the PR.

Also applies to: 886-886, 1127-1127, 1171-1171, 1178-1178

---

1204-1204: LGTM! contains() is more expressive than count().

Using contains() (C++20) instead of count() == 0 is more idiomatic and clearly expresses intent.

---

1062-1063: LGTM! Anonymous namespace appropriately scopes internal helpers.

Moving helper functions into anonymous namespaces prevents external linkage and is good practice for internal implementation details.

Also applies to: 1215-1215

src/blackboard.cpp (5)

9-15: LGTM! Anonymous namespace appropriately scopes helper.

Moving IsPrivateKey into an anonymous namespace is good practice and prevents naming conflicts.

---

47-47: LGTM! NOLINTNEXTLINE comments document necessary const_cast.

The const_cast operations are appropriately documented with NOLINTNEXTLINE comments, making it clear these are intentional design decisions required by the API design.

Also applies to: 326-326

---

61-61: Excellent const-correctness for read-only locks.

Marking locks as const in read-only critical sections is a great practice that prevents accidental modification of the lock object itself.

Also applies to: 139-139, 224-224

---

310-321: LGTM! Self-assignment guard is essential.

The self-assignment check in Entry::operator= prevents issues when assigning an object to itself and aligns with best practices.

---

174-177: LGTM! Clear intent with renamed variable and std::ignore.

Renaming the loop variable from _ to entry and using std::ignore makes the code more readable and explicitly documents that the entry value is unused in this loop.

.clang-tidy (3)

1-24: Well-balanced clang-tidy configuration.

The configuration enables comprehensive static analysis while pragmatically disabling checks that would generate excessive noise:

• -bugprone-easily-swappable-parameters: Reduces false positives for parameter ordering
• -cppcoreguidelines-avoid-magic-numbers: Avoids requiring named constants everywhere
• -misc-no-recursion: Allows recursion where appropriate (e.g., tree traversal)

This strikes a good balance between code quality enforcement and practical development.

---

27-27: Pragmatic exclusion of cognitive complexity from errors.

Excluding readability-function-cognitive-complexity from WarningsAsErrors is sensible, as complexity is often a judgment call and shouldn't block compilation. The configuration still reports complexity warnings, just doesn't escalate them to errors.

---

29-42: Well-documented CheckOptions with clear rationale.

Each check option includes a comment explaining why it's configured, which is excellent documentation:

• IgnoreMacros for RCLCPP LOG macros
• MISRA:2023 compliance note
• ROS API callback rationale for shared_ptr

This makes the configuration maintainable and understandable.

run_clang_tidy.sh (4)

1-1: LGTM! Strict bash flags improve script safety.

Using #!/bin/bash -eu ensures the script exits on errors and treats unset variables as errors, which prevents silent failures.

---

7-19: Excellent user experience for missing dependencies.

The script provides clear, actionable error messages with specific installation instructions when clangd-21 is missing. This significantly improves developer experience.

---

35-44: Maintainable skip list implementation.

The skip list approach makes it easy to add/remove paths to exclude, and the conversion to find arguments is clean.

---

62-72: Efficient parallel execution with good error handling.

Using xargs -P $(nproc) for parallel execution and set -o pipefail in the subprocess ensures efficient processing and proper error propagation. The per-file output prefixing is also helpful for debugging.

src/decorators/delay_node.cpp (2)

8-16: LGTM on constructor initialization.

Initializing timer_id_ to 0 in both constructors ensures deterministic state. The distinction between the two constructors (one with explicit milliseconds, one with config for port-based reading) is preserved correctly.

---

52-71: LGTM on tick() control flow.

The const lock and the restructured control flow correctly handle delay completion, child execution, and state reset. The emitWakeUpSignal() in the timer callback ensures the tree wakes up promptly when delay completes.

include/behaviortree_cpp/utils/simple_string.hpp (3)

21-25: LGTM on default constructor.

Properly initializes SOO storage with capacity and null-terminator, creating a valid empty string state.

---

133-153: LGTM on comparison operators.

The lexicographic comparison using memcmp with size-based tie-breaking is correct and follows standard string comparison semantics. Using const for local variables aligns with clang-tidy guidelines.

---

176-181: Union default initialization may cause issues with non-trivial members.

The union contains a String struct with default member initializers (data = nullptr, size = 0). In C++, a union with non-trivial members requires explicit constructor handling. The = {} initialization for _storage may not behave as expected across all compilers.

Consider verifying this compiles and behaves correctly on all target platforms.

src/tree_node.cpp (3)

57-66: LGTM on move constructor and defaulted destructor.

Using the member initializer list for the move constructor and defaulting the destructor is cleaner and follows modern C++ idioms.

---

309-328: LGTM on callback setters with move semantics.

The NOLINT comments correctly suppress the performance-unnecessary-value-param warning since the callbacks are intentionally taken by value and moved into storage. The const locks ensure thread-safe assignment.

---

388-392: Good fix: null-check before dereferencing.

Adding stripped_pointer != nullptr before dereferencing prevents undefined behavior when the caller passes a null output parameter.

src/behavior_tree.cpp (2)

21-24: LGTM on explicit nullptr comparisons.

Replacing !node with node == nullptr improves clarity and satisfies clang-tidy's readability checks.

Also applies to: 43-46, 75-79

---

101-102: LGTM on const-correctness for buildSerializedStatusSnapshot.

Taking const TreeNode* is appropriate since the function only reads node status without modification.

src/bt_factory.cpp (6)

105-105: LGTM on defaulted destructor.

Defaulting the destructor is appropriate for a class using the PImpl idiom with std::unique_ptr.

---

140-158: LGTM on const qualifiers for local variables.

Adding const to builder and manifest variables is good practice and aligns with clang-tidy recommendations.

---

179-184: LGTM on plugin symbol loading.

The using alias for the function pointer type is cleaner than a typedef. The NOLINT comment appropriately suppresses the reinterpret_cast warning which is unavoidable for dynamic library symbol loading.

---

251-280: LGTM on substitution rule handling.

Using const auto* const for the std::get_if results is correct and explicit. The NOLINT for readability-else-after-return at line 266 is appropriate since the else-if chain improves readability here.

---

420-420: LGTM on std::move for substitution rule.

Moving the rule avoids unnecessary copies of potentially complex SubstitutionRule variants.

---

477-477: LGTM on Tree cleanup changes.

Defaulting Tree() constructor, adding NOLINT for non-const member functions, and using !subtrees.empty() instead of size comparison are all good improvements.

Also applies to: 491-494, 552-552

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Quote $clang_tidy_paths to prevent word splitting.

Shellcheck reports SC2046 because $clang_tidy_paths is unquoted in the find command. If any path contains spaces, this will cause word splitting and unexpected behavior.

🔎 Proposed fix

-find $clang_tidy_paths \( -name '*.cpp' -o -name '*.hpp' \) -not -name '*_WIN.cpp' "${skip_paths[@]}" -print0 \
+find "$ws_dir/src" "$ws_dir/include" \( -name '*.cpp' -o -name '*.hpp' \) -not -name '*_WIN.cpp' "${skip_paths[@]}" -print0 \

Alternatively, if you want to keep the variable:

-clang_tidy_paths="$ws_dir/src $ws_dir/include"
+clang_tidy_paths=("$ws_dir/src" "$ws_dir/include")
...
-find $clang_tidy_paths \( -name '*.cpp' -o -name '*.hpp' \) -not -name '*_WIN.cpp' "${skip_paths[@]}" -print0 \
+find "${clang_tidy_paths[@]}" \( -name '*.cpp' -o -name '*.hpp' \) -not -name '*_WIN.cpp' "${skip_paths[@]}" -print0 \

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	find $clang_tidy_paths \( -name '*.cpp' -o -name '*.hpp' \) -not -name '*_WIN.cpp' "${skip_paths[@]}" -print0 \
	| xargs -0 -n 1 -P $(nproc) bash -c '
	find "$ws_dir/src" "$ws_dir/include" \( -name '*.cpp' -o -name '*.hpp' \) -not -name '*_WIN.cpp' "${skip_paths[@]}" -print0 \
	| xargs -0 -n 1 -P $(nproc) bash -c '

🧰 Tools

🪛 Shellcheck (0.11.0)

[warning] 62-62: Quote this to prevent word splitting.

(SC2046)

🤖 Prompt for AI Agents

In run_clang_tidy.sh around lines 61-62, the find command uses an unquoted
$clang_tidy_paths which can undergo word-splitting and break when paths contain
spaces; update the invocation to quote the variable (use "${clang_tidy_paths}"
if it's a single string of paths, or use the array expansion
"${clang_tidy_paths[@]}" if it is an array) so that each path is treated as a
single argument and shellcheck SC2046 is resolved.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/controls/manual_node.cpp (1)

86-124: Critical bug: Uninitialized window pointer used throughout function.

The window pointer win is initialized to nullptr (line 86) but is never assigned a valid window via newwin(). Subsequently, win is passed to multiple ncurses functions that require a valid WINDOW*:

• mvwprintw(win, ...) (lines 90-93)
• wrefresh(win) (line 95)
• keypad(win, TRUE) (line 97)
• wgetch(win) (line 119)
• werase(win) (line 121)
• wrefresh(win) (line 122)
• delwin(win) (line 123)

Passing nullptr to these functions will cause undefined behavior or crash. Compare with selectChild() which properly creates a window at line 149.

Either create a window with newwin() after initscr(), or use stdscr directly instead of win.

🔎 Proposed fix: Create window or use stdscr

Option 1: Create a window (consistent with selectChild)

 WINDOW* win = nullptr;
 initscr();
 cbreak();
+win = newwin(10, 70, 1, 1);  // create a new window

 mvwprintw(win, 0, 0, "No children.");

Option 2: Use stdscr directly

-WINDOW* win = nullptr;
 initscr();
 cbreak();

-mvwprintw(win, 0, 0, "No children.");
-mvwprintw(win, 1, 0, "Press: S to return SUCCESSFUL,");
-mvwprintw(win, 2, 0, "       F to return FAILURE, or");
-mvwprintw(win, 3, 0, "       R to return RUNNING.");
+mvwprintw(stdscr, 0, 0, "No children.");
+mvwprintw(stdscr, 1, 0, "Press: S to return SUCCESSFUL,");
+mvwprintw(stdscr, 2, 0, "       F to return FAILURE, or");
+mvwprintw(stdscr, 3, 0, "       R to return RUNNING.");

-wrefresh(win);
-noecho();
-keypad(win, TRUE);
-curs_set(0);
+wrefresh(stdscr);
+noecho();
+keypad(stdscr, TRUE);
+curs_set(0);

 // ... in the loop:
-ch = wgetch(win);
+ch = wgetch(stdscr);

 // ... at the end:
-werase(win);
-wrefresh(win);
-delwin(win);
+werase(stdscr);
+wrefresh(stdscr);
 endwin();

♻️ Duplicate comments (1)

src/xml_parsing.cpp (1)

1204-1204: Fix C++20-only contains() causing pipeline failure.

The call to temp_factory.builtinNodes().contains(registration_ID) uses std::set::contains() which is a C++20 feature, causing compilation failures in C++17 builds as shown in the pipeline logs.

🔎 Proposed fix

-    if(add_builtin_models || !temp_factory.builtinNodes().contains(registration_ID))
+    if(add_builtin_models || temp_factory.builtinNodes().count(registration_ID) == 0)

🧹 Nitpick comments (4)

src/controls/reactive_sequence.cpp (1)

58-58: Good modernization, but line 56 has the same C-style cast.

The change to static_cast<int>(index) improves type-safety and is consistent with modern C++ practices. However, line 56 still uses the C-style cast int(index) for the same conversion. For consistency, consider updating line 56 as well.

🔎 Suggested consistency fix for line 56

-        running_child_ = int(index);
+        running_child_ = static_cast<int>(index);

src/tree_node.cpp (1)

205-216: Consider: else-if made mutual exclusivity clearer.

While functionally equivalent (each branch returns), using else if would make it more explicit that these conditions are mutually exclusive. The current structure works correctly but is slightly less clear in intent.

Optional refactoring for clarity

 if(preID == PreCond::FAILURE_IF)
 {
   return NodeStatus::FAILURE;
 }
-if(preID == PreCond::SUCCESS_IF)
+else if(preID == PreCond::SUCCESS_IF)
 {
   return NodeStatus::SUCCESS;
 }
-if(preID == PreCond::SKIP_IF)
+else if(preID == PreCond::SKIP_IF)
 {
   return NodeStatus::SKIPPED;
 }

src/loggers/groot2_publisher.cpp (2)

118-118: Consider consistency with codebase style for lock declarations.

The const qualifier on std::unique_lock is valid when used purely for RAII scope-locking. However, looking at the relevant code snippets from other files (e.g., src/decorators/delay_node.cpp, src/loggers/bt_sqlite_logger.cpp), they use non-const locks: lk(delay_mutex_).

This is a minor stylistic inconsistency. The code is functionally correct since these locks don't call unlock()/lock() explicitly.

---

489-493: Variable name shadowing: inner lk shadows outer lk.

Line 493 declares a new lk that shadows the lk from line 489. While this works correctly (different scopes), it can cause confusion during maintenance. Consider renaming the inner lock (e.g., hook_lk) for clarity.

🔎 Suggested rename

   const std::unique_lock<std::mutex> lk(_p->hooks_map_mutex);
   for(const auto& [node_uid, hook] : _p->pre_hooks)
   {
     std::ignore = node_uid;  // unused in this loop
-    std::unique_lock<std::mutex> lk(hook->mutex);
+    std::unique_lock<std::mutex> hook_lk(hook->mutex);
     hook->enabled = enable;
     // when disabling, remember to wake up blocked ones
     if(!hook->enabled && hook->mode == Monitor::Hook::Mode::BREAKPOINT)
     {
-      lk.unlock();
+      hook_lk.unlock();
       hook->wakeup.notify_all();
     }
   }

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f1906d8 and 0bd6710.

📒 Files selected for processing (97)

• .clang-tidy
• .gitignore
• .pre-commit-config.yaml
• include/behaviortree_cpp/action_node.h
• include/behaviortree_cpp/actions/always_failure_node.h
• include/behaviortree_cpp/actions/always_success_node.h
• include/behaviortree_cpp/actions/pop_from_queue.hpp
• include/behaviortree_cpp/actions/script_condition.h
• include/behaviortree_cpp/actions/script_node.h
• include/behaviortree_cpp/actions/set_blackboard_node.h
• include/behaviortree_cpp/actions/sleep_node.h
• include/behaviortree_cpp/actions/test_node.h
• include/behaviortree_cpp/actions/unset_blackboard_node.h
• include/behaviortree_cpp/actions/updated_action.h
• include/behaviortree_cpp/behavior_tree.h
• include/behaviortree_cpp/bt_factory.h
• include/behaviortree_cpp/bt_parser.h
• include/behaviortree_cpp/condition_node.h
• include/behaviortree_cpp/control_node.h
• include/behaviortree_cpp/controls/fallback_node.h
• include/behaviortree_cpp/controls/if_then_else_node.h
• include/behaviortree_cpp/controls/manual_node.h
• include/behaviortree_cpp/controls/parallel_all_node.h
• include/behaviortree_cpp/controls/parallel_node.h
• include/behaviortree_cpp/controls/reactive_fallback.h
• include/behaviortree_cpp/controls/reactive_sequence.h
• include/behaviortree_cpp/controls/sequence_node.h
• include/behaviortree_cpp/controls/sequence_with_memory_node.h
• include/behaviortree_cpp/controls/switch_node.h
• include/behaviortree_cpp/controls/while_do_else_node.h
• include/behaviortree_cpp/decorators/consume_queue.h
• include/behaviortree_cpp/decorators/delay_node.h
• include/behaviortree_cpp/decorators/force_failure_node.h
• include/behaviortree_cpp/decorators/force_success_node.h
• include/behaviortree_cpp/decorators/inverter_node.h
• include/behaviortree_cpp/decorators/keep_running_until_failure_node.h
• include/behaviortree_cpp/decorators/loop_node.h
• include/behaviortree_cpp/decorators/repeat_node.h
• include/behaviortree_cpp/decorators/retry_node.h
• include/behaviortree_cpp/decorators/run_once_node.h
• include/behaviortree_cpp/decorators/script_precondition.h
• include/behaviortree_cpp/decorators/timeout_node.h
• include/behaviortree_cpp/decorators/updated_decorator.h
• include/behaviortree_cpp/exceptions.h
• include/behaviortree_cpp/leaf_node.h
• include/behaviortree_cpp/scripting/any_types.hpp
• include/behaviortree_cpp/scripting/operators.hpp
• include/behaviortree_cpp/scripting/script_parser.hpp
• include/behaviortree_cpp/tree_node.h
• include/behaviortree_cpp/utils/convert_impl.hpp
• include/behaviortree_cpp/utils/locked_reference.hpp
• include/behaviortree_cpp/utils/safe_any.hpp
• include/behaviortree_cpp/utils/simple_string.hpp
• include/behaviortree_cpp/utils/wildcards.hpp
• run_clang_tidy.sh
• src/action_node.cpp
• src/actions/sleep_node.cpp
• src/actions/updated_action.cpp
• src/basic_types.cpp
• src/behavior_tree.cpp
• src/blackboard.cpp
• src/bt_factory.cpp
• src/condition_node.cpp
• src/control_node.cpp
• src/controls/fallback_node.cpp
• src/controls/if_then_else_node.cpp
• src/controls/manual_node.cpp
• src/controls/parallel_all_node.cpp
• src/controls/parallel_node.cpp
• src/controls/reactive_fallback.cpp
• src/controls/reactive_sequence.cpp
• src/controls/sequence_node.cpp
• src/controls/sequence_with_memory_node.cpp
• src/controls/switch_node.cpp
• src/controls/while_do_else_node.cpp
• src/decorator_node.cpp
• src/decorators/delay_node.cpp
• src/decorators/inverter_node.cpp
• src/decorators/repeat_node.cpp
• src/decorators/retry_node.cpp
• src/decorators/subtree_node.cpp
• src/decorators/timeout_node.cpp
• src/decorators/updated_decorator.cpp
• src/example.cpp
• src/json_export.cpp
• src/loggers/bt_cout_logger.cpp
• src/loggers/bt_file_logger_v2.cpp
• src/loggers/bt_minitrace_logger.cpp
• src/loggers/bt_sqlite_logger.cpp
• src/loggers/groot2_publisher.cpp
• src/script_parser.cpp
• src/shared_library.cpp
• src/shared_library_UNIX.cpp
• src/tree_node.cpp
• src/xml_parsing.cpp
• tests/CMakeLists.txt
• tests/gtest_simple_string.cpp

💤 Files with no reviewable changes (1)

• src/example.cpp

✅ Files skipped from review due to trivial changes (47)

• include/behaviortree_cpp/bt_parser.h
• include/behaviortree_cpp/controls/reactive_sequence.h
• include/behaviortree_cpp/decorators/delay_node.h
• src/controls/parallel_node.cpp
• include/behaviortree_cpp/behavior_tree.h
• include/behaviortree_cpp/decorators/repeat_node.h
• include/behaviortree_cpp/actions/test_node.h
• include/behaviortree_cpp/controls/while_do_else_node.h
• include/behaviortree_cpp/controls/fallback_node.h
• include/behaviortree_cpp/controls/sequence_with_memory_node.h
• include/behaviortree_cpp/decorators/loop_node.h
• include/behaviortree_cpp/controls/parallel_node.h
• include/behaviortree_cpp/decorators/updated_decorator.h
• include/behaviortree_cpp/decorators/force_failure_node.h
• include/behaviortree_cpp/actions/unset_blackboard_node.h
• include/behaviortree_cpp/decorators/script_precondition.h
• include/behaviortree_cpp/actions/script_condition.h
• include/behaviortree_cpp/condition_node.h
• include/behaviortree_cpp/controls/manual_node.h
• src/controls/sequence_with_memory_node.cpp
• include/behaviortree_cpp/actions/updated_action.h
• include/behaviortree_cpp/controls/if_then_else_node.h
• include/behaviortree_cpp/action_node.h
• include/behaviortree_cpp/decorators/timeout_node.h
• include/behaviortree_cpp/tree_node.h
• include/behaviortree_cpp/actions/script_node.h
• include/behaviortree_cpp/decorators/run_once_node.h
• include/behaviortree_cpp/decorators/force_success_node.h
• include/behaviortree_cpp/actions/set_blackboard_node.h
• include/behaviortree_cpp/bt_factory.h
• include/behaviortree_cpp/scripting/any_types.hpp
• include/behaviortree_cpp/scripting/script_parser.hpp
• include/behaviortree_cpp/scripting/operators.hpp
• include/behaviortree_cpp/decorators/retry_node.h
• include/behaviortree_cpp/controls/switch_node.h
• include/behaviortree_cpp/actions/always_success_node.h
• tests/gtest_simple_string.cpp
• include/behaviortree_cpp/actions/always_failure_node.h
• include/behaviortree_cpp/controls/parallel_all_node.h
• include/behaviortree_cpp/control_node.h
• src/decorators/inverter_node.cpp
• include/behaviortree_cpp/controls/reactive_fallback.h
• include/behaviortree_cpp/decorators/consume_queue.h
• include/behaviortree_cpp/leaf_node.h
• src/condition_node.cpp
• src/controls/if_then_else_node.cpp
• include/behaviortree_cpp/exceptions.h

🚧 Files skipped from review as they are similar to previous changes (20)

• src/json_export.cpp
• src/shared_library_UNIX.cpp
• src/control_node.cpp
• src/controls/fallback_node.cpp
• src/loggers/bt_sqlite_logger.cpp
• src/actions/sleep_node.cpp
• src/decorator_node.cpp
• src/decorators/repeat_node.cpp
• include/behaviortree_cpp/utils/safe_any.hpp
• .clang-tidy
• src/controls/switch_node.cpp
• src/loggers/bt_minitrace_logger.cpp
• run_clang_tidy.sh
• src/controls/while_do_else_node.cpp
• src/controls/parallel_all_node.cpp
• src/decorators/updated_decorator.cpp
• .gitignore
• include/behaviortree_cpp/utils/simple_string.hpp
• src/decorators/subtree_node.cpp
• src/controls/reactive_fallback.cpp

🧰 Additional context used

🧬 Code graph analysis (11)

src/actions/updated_action.cpp (5)

src/actions/sleep_node.cpp (1)

• lk (28-28)

src/decorators/delay_node.cpp (2)

• lk (43-43)
• lk (52-52)

src/decorators/timeout_node.cpp (2)

• lk (44-44)
• lk (55-55)

src/loggers/bt_sqlite_logger.cpp (2)

• lk (189-189)
• lk (208-208)

src/loggers/groot2_publisher.cpp (2)

• lk (118-118)
• lk (198-198)

src/loggers/bt_cout_logger.cpp (1)

include/behaviortree_cpp/loggers/bt_cout_logger.h (1)

• StdCoutLogger (14-25)

src/shared_library.cpp (1)

include/behaviortree_cpp/exceptions.h (1)

• RuntimeError (58-67)

src/loggers/groot2_publisher.cpp (5)

src/actions/sleep_node.cpp (1)

• lk (28-28)

src/decorators/delay_node.cpp (2)

• lk (43-43)
• lk (52-52)

src/decorators/timeout_node.cpp (2)

• lk (44-44)
• lk (55-55)

src/loggers/bt_sqlite_logger.cpp (2)

• lk (189-189)
• lk (208-208)

src/tree_node.cpp (2)

• status (276-280)
• status (276-276)

src/script_parser.cpp (2)

include/behaviortree_cpp/scripting/any_types.hpp (1)

• result (33-41)

tests/gtest_preconditions.cpp (1)

• i (290-298)

src/tree_node.cpp (1)

include/behaviortree_cpp/tree_node.h (1)

• TreeNode (131-295)

src/controls/sequence_node.cpp (1)

src/tree_node.cpp (2)

• setRegistrationID (433-436)
• setRegistrationID (433-433)

src/loggers/bt_file_logger_v2.cpp (2)

src/tree_node.cpp (2)

• status (276-280)
• status (276-276)

src/shared_library_UNIX.cpp (3)

• lock (13-13)
• lock (32-32)
• lock (48-48)

src/decorators/delay_node.cpp (1)

include/behaviortree_cpp/decorators/delay_node.h (1)

• DelayNode (34-66)

src/action_node.cpp (2)

include/behaviortree_cpp/action_node.h (1)

• CoroActionNode (196-229)

3rdparty/minicoro/minicoro.h (2)

• mco_create (1684-1711)
• mco_destroy (1713-1729)

src/decorators/timeout_node.cpp (3)

src/decorator_node.cpp (6)

• child (38-41)
• child (38-38)
• child (43-46)
• child (43-43)
• resetChild (53-64)
• resetChild (53-53)

include/behaviortree_cpp/basic_types.h (1)

• isStatusCompleted (47-50)

src/decorators/delay_node.cpp (2)

• lk (43-43)
• lk (52-52)

🪛 GitHub Actions: cmake Ubuntu

src/xml_parsing.cpp

[error] 1204-1204: error: 'const class std::set<std::__cxx11::basic_string >' has no member named 'contains' (use 'count' or 'find' instead)

🪛 GitHub Actions: cmake Ubuntu Sanitizers

src/xml_parsing.cpp

[error] 1204-1204: Compilation error: 'std::set<...>::contains' is not a member in this C++ version; use find() or count() instead. Code references 'temp_factory.builtinNodes().contains(registration_ID)'.

---

[error] Compilation halted due to previous error in xml_parsing.cpp during 'cmake --build --preset conan-debug'.

🔇 Additional comments (87)

src/loggers/bt_cout_logger.cpp (2)

8-8: LGTM! Good modernization.

Defaulting the destructor is the correct approach when no custom cleanup is needed, aligning with modern C++ best practices.

---

18-18: LGTM! Good const-correctness improvement.

The variable is correctly qualified as const since it's never modified after initialization.

include/behaviortree_cpp/actions/sleep_node.h (1)

38-38: LGTM! Good initialization practice.

Initializing timer_id_ to 0 at declaration prevents undefined behavior and aligns with modern C++ best practices. This change is consistent with the existing initialization of timer_waiting_ and is likely a clang-tidy fix for uninitialized member variables.

include/behaviortree_cpp/controls/sequence_node.h (1)

1-12: LGTM!

The copyright year update and license text corrections are appropriate maintenance changes with no functional impact.

src/shared_library.cpp (1)

12-15: LGTM! Improved readability with explicit nullptr check and braces.

The explicit nullptr comparison and added braces align with modern C++ best practices and improve code clarity. The control flow remains functionally identical, and the change is consistent with the explicit nullptr check already present in hasSymbol at Line 21.

src/controls/sequence_node.cpp (1)

22-29: LGTM! Good style improvement.

Adding explicit braces around both branches improves consistency and prevents potential issues if additional statements are added later. This aligns with clang-tidy best practices.

src/script_parser.cpp (3)

26-26: LGTM: const-correctness improvements.

The const qualifier is correctly applied since the exprs vector is never modified after initialization. In ParseScript, the lambda captures by value, so the const on the original doesn't affect the captured copy.

Also applies to: 78-78

---

35-35: LGTM: Style consistency for unsigned literal.

Using uppercase 0U is the more conventional form and aligns with common style guidelines.

---

52-52: LGTM: Simplified control flow by removing redundant else blocks.

Removing explicit else after early returns reduces nesting and improves readability. This follows the clang-tidy readability-else-after-return recommendation.

Also applies to: 62-63

src/loggers/bt_file_logger_v2.cpp (6)

8-14: LGTM!

Good use of anonymous namespace to give ToUsec internal linkage. This prevents potential ODR violations and allows better compiler optimization.

---

57-59: LGTM!

Good modernization: std::array<char, 8> with value initialization provides type safety and guarantees zero-initialization, eliminating potential UB from uninitialized memory.

---

62-62: LGTM!

Explicit std::streamsize cast is correct for ostream::write and silences sign-conversion warnings from clang-tidy.

---

67-69: LGTM!

Correct 8-byte buffer usage for writing the 64-bit timestamp.

---

85-91: LGTM!

Value initialization of Transition trans{} ensures all members are properly initialized. The const std::scoped_lock is correct since the lock object itself isn't modified after construction.

---

119-126: LGTM!

Good modernization to std::array<char, 9> with value initialization. The buffer size correctly matches the 9-byte record format (6 + 2 + 1 bytes).

Note: The partial memcpy of larger integer types (e.g., 6 bytes from uint64_t) assumes little-endian byte order—this is pre-existing behavior, not introduced by this change.

src/decorators/retry_node.cpp (3)

2-2: LGTM: Copyright year updated.

The copyright year update to 2025 is appropriate and routine maintenance.

---

55-56: LGTM: Excellent const-correctness improvement.

Adding const to prev_status and child_status is good practice since these variables are never modified after initialization. This improves code clarity and prevents accidental modification.

---

44-46: Verify NUM_ATTEMPTS is properly defined in the header.

The AI summary indicates that NUM_ATTEMPTS static member declaration was removed from this class. The code still uses NUM_ATTEMPTS at lines 44, 46, and 69. Ensure that NUM_ATTEMPTS is properly defined as an inline constexpr in the header file (src/main/include/behaviortree_cpp/decorators/retry_node.h) to maintain accessibility and prevent compilation errors.

src/blackboard.cpp (7)

2-2: LGTM! Good modernization practices.

The <tuple> include is correctly added for std::ignore usage at line 176, and moving IsPrivateKey into an anonymous namespace properly limits its linkage to this translation unit, preventing potential ODR violations.

Also applies to: 9-15

---

47-48: LGTM! Appropriate suppression.

The NOLINTNEXTLINE suppression is justified here. The const_cast is safe and necessary for the implementation pattern where the non-const overload delegates to the const version.

---

61-61: Verify const qualification on lock objects.

Making std::unique_lock objects const is unusual. While technically correct (the destructor can still unlock a const lock object), standard practice typically leaves lock objects non-const. This appears to be a clang-tidy suggestion for const-correctness, but the practical benefit is minimal since these locks are never reassigned.

If this is a project-wide convention from clang-tidy, it's acceptable. Otherwise, consider whether this deviates from typical C++ mutex usage patterns for marginal gain.

Also applies to: 139-139, 224-224

---

174-176: LGTM! Better unused variable handling.

Using std::ignore = entry; with an explicit comment is clearer and more standard than using _ as the variable name. This properly suppresses unused variable warnings in a portable way.

---

281-281: LGTM! Good const-correctness.

Making name const is a simple improvement that clearly indicates the variable won't be modified after initialization.

---

312-320: LGTM! Essential best practice.

The self-assignment guard is a recommended best practice for assignment operators. While the members likely handle self-assignment safely, this guard:

• Prevents unnecessary work when x = x
• Provides defensive programming against edge cases
• Improves performance by short-circuiting the copy

---

326-327: LGTM! Appropriate suppression.

Similar to line 47, this NOLINTNEXTLINE suppression is justified. The const_cast is safe and necessary for the implementation pattern where the non-const overload delegates to the const version.

src/controls/manual_node.cpp (3)

76-76: LGTM: Const-correctness improvement.

The addition of const is appropriate since the variable is assigned once and never modified.

---

145-149: LGTM: Proper initialization and type-safe conversions.

The changes demonstrate good practices:

• Line 145-149: Pointer initialized to nullptr then properly assigned via newwin()
• Lines 149, 159: Explicit static_cast<int>() conversions make type conversions clear and safe
• Line 159: Format specifier %2zu is correct for size_t type
• Line 169: while(true) is more idiomatic C++ than while(1)

Also applies to: 159-159, 169-169

---

101-102: LGTM: Proper initialization.

Initializing ret to a default value (RUNNING) before the loop is good defensive programming. The while(true) construct is also more idiomatic than while(1).

include/behaviortree_cpp/decorators/keep_running_until_failure_node.h (1)

1-1: LGTM!

Copyright year update is appropriate.

include/behaviortree_cpp/decorators/inverter_node.h (1)

2-2: LGTM!

Copyright year update is appropriate.

.pre-commit-config.yaml (1)

46-54: Commented-out clang-tidy hook provides useful documentation.

The commented configuration serves as a reference for contributors who want to enable clang-tidy locally. This is reasonable given that clang-tidy requires a compile_commands.json in the build directory, which may not always be present.

Consider adding a brief comment explaining why it's commented out (e.g., "requires a build directory with compile_commands.json") to help future contributors.

include/behaviortree_cpp/utils/wildcards.hpp (2)

3-7: LGTM!

Adding explicit includes for the types used (size_t, int8_t, std::string_view, std::vector) improves portability and aligns with best practices for header self-containment.

---

43-43: Good fix for uninitialized variable warning.

Explicitly initializing result to false eliminates the clang-tidy warning about potentially uninitialized variables, even though all code paths ultimately assign a value.

src/actions/updated_action.cpp (2)

1-1: LGTM!

Copyright year update is appropriate.

---

43-43: Good const-correctness improvement.

Adding const to the lock is appropriate since the lock object itself isn't modified after construction. This aligns with clang-tidy's const-correctness recommendations.

include/behaviortree_cpp/actions/pop_from_queue.hpp (3)

1-1: LGTM!

Copyright year update is appropriate.

---

74-79: LGTM!

The restructured control flow is cleaner and functionally equivalent. Early return on empty queue, then the happy path proceeds linearly.

---

118-125: Verify the behavioral change for empty queues.

The previous implementation likely returned FAILURE for empty queues. The new structure maintains this behavior, but consider whether an empty queue should return SUCCESS with size=0 instead of FAILURE.

If returning FAILURE for empty queues is intentional (e.g., to signal "nothing to process"), the current behavior is correct.

include/behaviortree_cpp/utils/convert_impl.hpp (2)

1-1: LGTM!

Copyright year update is appropriate.

---

157-171: Verify the asymmetric handling of float→bool vs int→bool conversions.

The new logic treats these conversions differently:

• Float→bool (lines 161-165): Any non-zero value becomes true (C-style semantics).
• Int→bool (lines 167-170): Only 0 or 1 are valid; other values throw via checkLowerLimit.

This means convertNumber(2.0f, boolVal) succeeds (→ true), but convertNumber(2, boolVal) throws. If this asymmetry is intentional (e.g., stricter integer validation), the implementation is correct. Otherwise, consider applying consistent rules to both numeric types.

include/behaviortree_cpp/utils/locked_reference.hpp (4)

28-31: LGTM!

Using explicit mutex_ != nullptr instead of implicit boolean conversion is clearer and satisfies clang-tidy's explicit comparison checks.

---

37-41: LGTM!

Adding noexcept to the move constructor is correct since std::swap on pointers is noexcept. This enables better optimization and is a best practice for move operations.

---

43-48: LGTM!

Adding noexcept to the move assignment operator is appropriate. The swap-based implementation only operates on pointers, which cannot throw.

---

55-69: LGTM!

Consistent use of explicit mutex_ != nullptr checks in lock() and unlock() methods.

src/action_node.cpp (3)

75-81: LGTM: Good use of anonymous namespace for internal linkage.

Wrapping CoroEntry in an anonymous namespace provides internal linkage, preventing potential symbol collisions across translation units.

---

89-96: LGTM: Exception-safe destructor implementation.

The try/catch block prevents exceptions from escaping the destructor, which would cause program termination. Logging to std::cerr is appropriate for destructor failures.

---

148-156: LGTM: Explicit nullptr check improves safety.

The explicit check _p->coro != nullptr before calling mco_destroy prevents potential issues and makes the code more defensive.

src/xml_parsing.cpp (2)

89-94: LGTM: Anonymous namespace for internal helper.

Using an anonymous namespace for the StrEqual lambda provides internal linkage and prevents naming conflicts.

---

121-129: LGTM: Factory pointer conversion improves const-correctness.

Converting the factory from a reference to a const BehaviorTreeFactory* with pointer-based access throughout the class is a good refactor that maintains immutability guarantees.

src/bt_factory.cpp (3)

109-109: LGTM: C++17-compatible implementation.

Using count() != 0 instead of contains() ensures C++17 compatibility, unlike the issue in xml_parsing.cpp line 1204.

---

140-146: LGTM: Const-correctness for builder and manifest.

Marking both builder and manifest as const prevents accidental modifications and aligns with best practices for lambda captures and registration flow.

---

420-420: LGTM: Efficient use of move semantics.

Using std::move(rule) avoids unnecessary copies when inserting into the substitution rules map.

tests/CMakeLists.txt (1)

32-32: LGTM: Test coverage for SimpleString.

Adding gtest_simple_string.cpp to the test suite provides coverage for the SimpleString utility class.

src/basic_types.cpp (4)

50-72: Switch statement improves maintainability.

Replacing nested if-else with an explicit switch statement makes the colored status logic more readable and maintainable.

Note: The color comments on lines 55 and 59 are swapped (already flagged in previous review).

---

143-156: LGTM: Anonymous namespace for internal helper.

Placing the ConvertWithBoundCheck template in an anonymous namespace provides proper internal linkage for this implementation detail.

---

200-206: LGTM: Thread-safe locale handling with const-correctness.

The const qualifiers on old_locale, str_copy, and val improve const-correctness. Proper locale save/restore ensures thread-safe string-to-double conversion.

---

447-447: LGTM: Cast prevents undefined behavior.

Casting to unsigned char before calling std::isalpha prevents sign-extension issues when char is signed, which would cause undefined behavior for values outside [0, 127].

src/tree_node.cpp (6)

57-58: LGTM: Move constructor modernized.

Using the member initializer list for the move constructor is better practice than body initialization.

---

66-66: LGTM: Defaulted destructor is appropriate.

Since _p is a unique_ptr, defaulting the destructor is correct and allows the compiler to generate optimal cleanup code.

---

160-160: LGTM: Explicit initialization of prev_status.

Explicitly initializing prev_status to NodeStatus::IDLE is good defensive programming and prevents potential undefined behavior.

Also applies to: 261-261

---

197-197: LGTM: static_cast is safer than C-style cast.

Using static_cast is more explicit and type-safe than C-style casting.

---

309-328: LGTM: Pass-by-value + move pattern is correct.

The NOLINTNEXTLINE suppressions are justified. The pass-by-value followed by std::move pattern is actually the recommended approach for move-enabled types like std::function, as it efficiently handles both lvalue and rvalue arguments without requiring two overloads. The locking ensures thread-safe callback updates.

---

388-391: LGTM: Explicit nullptr check improves clarity.

The explicit != nullptr comparison is clearer and more self-documenting than implicit pointer-to-bool conversion.

src/behavior_tree.cpp (6)

21-21: Explicit nullptr comparison improves readability.

The change from !node to node == nullptr follows modern C++ best practices and clang-tidy recommendations.

---

43-43: Explicit nullptr comparison improves readability.

The change from !node to node == nullptr follows modern C++ best practices and clang-tidy recommendations.

---

59-62: Defensive null-check added for decorator child.

The added guard prevents recursing into a null decorator child. However, this creates a behavioral difference from the const overload at line 37, which lacks this check. See the comment on lines 35-38 for details.

---

75-75: Explicit nullptr comparison improves readability.

The change from !node to node == nullptr follows modern C++ best practices and clang-tidy recommendations.

---

101-102: Const-correctness improvement for read-only function parameter.

Adding const to the TreeNode* parameter is appropriate. This change is source-compatible in C++ (callers can pass non-const pointers to const parameters), so it's not a breaking API change. Please verify that the function implementation only performs read operations on the parameter and that all call sites remain unaffected.

---

35-38: Unable to verify the specific inconsistency without access to the actual code at lines 35-38 and 59-62 of src/behavior_tree.cpp. The repository could not be cloned, and web searches confirm that both const and non-const overloads of applyRecursiveVisitor exist, and that DecoratorNode's child() method can return nullptr (per the library's recommended defensive null-checking pattern). However, the exact code structure and presence or absence of null guards in each overload cannot be confirmed.

Manual code inspection is required to verify whether the non-const overload at lines 59-62 actually contains a null guard for decorator->child() while the const overload at lines 35-38 lacks it, or whether both overloads have consistent handling.

src/loggers/groot2_publisher.cpp (12)

4-4: LGTM!

The <tuple> include is correctly added to support std::ignore usage throughout the file.

---

33-57: LGTM!

Good improvements:

• Anonymous namespace provides internal linkage for this helper function
• std::random_device seeding improves randomness quality over a fixed seed
• Value initialization out{} ensures zero-initialization
• Explicit static_cast<char> for 0xBF and 0x80 is correct since these values exceed the signed char range (127), avoiding implementation-defined behavior

---

66-72: LGTM!

Adding const to these configuration variables improves code safety and intent clarity.

---

103-103: LGTM!

Value initialization {} is a safer default for duration types.

---

212-212: LGTM!

The explicit cast chain clarifies the intent: enum to int for arithmetic, then to char for storage.

---

218-218: LGTM!

Value initialization {} zeroes the entire struct including padding bytes, which is important for consistent serialization behavior.

---

364-367: LGTM!

Good const-correctness on these parsed values that are only read, not modified.

---

419-423: LGTM!

Using std::ignore to explicitly mark unused structured binding variables is a clean way to suppress compiler warnings while documenting intent.

---

513-513: LGTM!

Adding const clarifies this is a snapshot value used for comparison.

---

554-554: LGTM!

Making the TreeNode::Ptr (shared_ptr) const prevents reassignment while still allowing operations on the pointed-to node.

---

684-692: LGTM!

Using auto* explicitly shows pointer intent, and std::ignore properly marks the unused hook_ptr variable.

---

706-707: LGTM!

Consistent with the style improvements made elsewhere in the file.

src/decorators/timeout_node.cpp (2)

44-44: LGTM on const lock qualifier.

The const on the unique_lock correctly signals that this lock is never explicitly unlocked in the callback, improving code clarity.

---

62-71: Well-structured refactor of the completion flow.

Capturing child_status once and then handling the completion path is cleaner than the previous nested logic. The unlock/cancel/relock pattern correctly avoids holding the mutex during timer cancellation while ensuring resetChild() runs under the lock.

src/decorators/delay_node.cpp (4)

9-9: Good: explicit initialization of timer_id_.

Initializing timer_id_ to 0 in both constructors ensures defined state before first use.

Also applies to: 15-15

---

43-43: Consistent use of const lock qualifiers.

Both locks are correctly marked const since neither is explicitly unlocked before scope exit, matching the pattern in timeout_node.cpp.

Also applies to: 52-52

---

60-71: Clean control flow for delay completion handling.

The logic correctly executes the child only after delay completes, resets state on child completion, and falls through to return RUNNING when delay is still pending.

---

54-58: delay_aborted_ is set to true in the halt() method.

The review's conclusion appears to be incorrect. According to the BehaviorTree.CPP documentation, the halt() implementation does set delay_aborted_ to signal that the delay operation was aborted. This flag is used to stop the timer/wait loop, allowing the node to exit the RUNNING state early. Therefore, the condition at line 54 is not dead code—it is a valid code path that executes when halt() is called on a running delay node.

Likely an incorrect or invalid review comment.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (2)

src/xml_parsing.cpp (1)

693-695: Variable name typo: is_blacbkboard should be is_blackboard.

This typo was already flagged in a previous review. The variable name should be corrected to is_blackboard to fix the spelling and improve readability.

run_clang_tidy.sh (1)

62-63: Quote $clang_tidy_paths to prevent word splitting.

The variable $clang_tidy_paths is unquoted, which can cause issues if paths contain spaces. This was flagged in a previous review.

🔎 Proposed fix using array

-clang_tidy_paths="$ws_dir/src $ws_dir/include"
+clang_tidy_paths=("$ws_dir/src" "$ws_dir/include")
...
-find $clang_tidy_paths \( -name '*.cpp' -o -name '*.hpp' -o -name '*.h' \) -not -name '*_WIN.cpp' "${skip_paths[@]}" -print0 \
+find "${clang_tidy_paths[@]}" \( -name '*.cpp' -o -name '*.hpp' -o -name '*.h' \) -not -name '*_WIN.cpp' "${skip_paths[@]}" -print0 \

🧹 Nitpick comments (5)

src/xml_parsing.cpp (1)

207-211: Consider std::array for type safety.

The C-style array works correctly, but using std::array would provide compile-time size checking and better type safety.

🔎 Proposed refactor

-      const std::pair<const char*, BT::PortDirection> port_types[3] = {
+      const std::array<std::pair<const char*, BT::PortDirection>, 3> port_types = {{
         { "input_port", BT::PortDirection::INPUT },
         { "output_port", BT::PortDirection::OUTPUT },
         { "inout_port", BT::PortDirection::INOUT }
-      };
+      }};

.clang-tidy (2)

27-27: Minor: Potential redundancy in check configuration.

The check misc-non-private-member-variables-in-classes is disabled at line 27 (-misc-non-private-member-variables-in-classes), but there's also a CheckOptions entry at lines 38-40 configuring IgnoreClassesWithAllMemberVariablesBeingPublic for the same check. Since the check is disabled, the option has no effect.

Consider either:

• Removing the disable entry if you want the check enabled with the custom option, or
• Removing the CheckOptions entry if the check should remain fully disabled.

Also applies to: 38-40

---

48-48: Incomplete reference comment.

Line 48 references the identifier-naming documentation but no corresponding CheckOptions are configured. If this is a placeholder for future configuration, consider adding a TODO comment or removing the incomplete reference.

include/behaviortree_cpp/action_node.h (2)

131-153: Consider adding explicit deleted copy/move operations to ThreadedAction for consistency.

ThreadedAction is implicitly non-copyable due to std::future and std::mutex members, but other action node classes in this file now explicitly delete these operations. Explicit deletion improves code clarity and produces better error messages.

Suggested addition after the constructor

   ThreadedAction(const std::string& name, const NodeConfig& config)
     : ActionNodeBase(name, config)
   {}
+
+  ThreadedAction(const ThreadedAction&) = delete;
+  ThreadedAction& operator=(const ThreadedAction&) = delete;
+  ThreadedAction(ThreadedAction&&) = delete;
+  ThreadedAction& operator=(ThreadedAction&&) = delete;

   bool isHaltRequested() const

---

174-202: Consider adding explicit deleted copy/move operations to StatefulActionNode for consistency.

Similar to ThreadedAction, this class would benefit from explicit deletion to match the pattern applied to other action node classes in this file.

Suggested addition after the constructor

   StatefulActionNode(const std::string& name, const NodeConfig& config)
     : ActionNodeBase(name, config)
   {}
+
+  StatefulActionNode(const StatefulActionNode&) = delete;
+  StatefulActionNode& operator=(const StatefulActionNode&) = delete;
+  StatefulActionNode(StatefulActionNode&&) = delete;
+  StatefulActionNode& operator=(StatefulActionNode&&) = delete;

   /// Method called once, when transitioning from the state IDLE.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0bd6710 and db1b06f.

📒 Files selected for processing (41)

• .clang-tidy
• .gitignore
• CMakeLists.txt
• include/behaviortree_cpp/action_node.h
• include/behaviortree_cpp/actions/sleep_node.h
• include/behaviortree_cpp/actions/updated_action.h
• include/behaviortree_cpp/blackboard.h
• include/behaviortree_cpp/condition_node.h
• include/behaviortree_cpp/control_node.h
• include/behaviortree_cpp/controls/fallback_node.h
• include/behaviortree_cpp/controls/if_then_else_node.h
• include/behaviortree_cpp/controls/manual_node.h
• include/behaviortree_cpp/controls/parallel_all_node.h
• include/behaviortree_cpp/controls/parallel_node.h
• include/behaviortree_cpp/controls/sequence_node.h
• include/behaviortree_cpp/controls/sequence_with_memory_node.h
• include/behaviortree_cpp/controls/switch_node.h
• include/behaviortree_cpp/controls/while_do_else_node.h
• include/behaviortree_cpp/decorator_node.h
• include/behaviortree_cpp/decorators/delay_node.h
• include/behaviortree_cpp/decorators/inverter_node.h
• include/behaviortree_cpp/decorators/repeat_node.h
• include/behaviortree_cpp/decorators/retry_node.h
• include/behaviortree_cpp/decorators/script_precondition.h
• include/behaviortree_cpp/decorators/subtree_node.h
• include/behaviortree_cpp/decorators/timeout_node.h
• include/behaviortree_cpp/decorators/updated_decorator.h
• include/behaviortree_cpp/json_export.h
• include/behaviortree_cpp/leaf_node.h
• include/behaviortree_cpp/loggers/bt_cout_logger.h
• include/behaviortree_cpp/loggers/bt_minitrace_logger.h
• include/behaviortree_cpp/loggers/bt_observer.h
• include/behaviortree_cpp/loggers/bt_sqlite_logger.h
• include/behaviortree_cpp/loggers/groot2_protocol.h
• include/behaviortree_cpp/utils/demangle_util.h
• include/behaviortree_cpp/utils/shared_library.h
• include/behaviortree_cpp/utils/timer_queue.h
• run_clang_tidy.sh
• src/blackboard.cpp
• src/controls/parallel_all_node.cpp
• src/xml_parsing.cpp

🚧 Files skipped from review as they are similar to previous changes (9)

• include/behaviortree_cpp/actions/updated_action.h
• .gitignore
• include/behaviortree_cpp/decorators/updated_decorator.h
• src/blackboard.cpp
• include/behaviortree_cpp/decorators/retry_node.h
• include/behaviortree_cpp/controls/manual_node.h
• include/behaviortree_cpp/decorators/timeout_node.h
• include/behaviortree_cpp/leaf_node.h
• include/behaviortree_cpp/controls/fallback_node.h

🧰 Additional context used

🧬 Code graph analysis (19)

include/behaviortree_cpp/utils/shared_library.h (3)

src/shared_library.cpp (1)

• SharedLibrary (4-7)

src/shared_library_UNIX.cpp (1)

• SharedLibrary (9-9)

src/shared_library_WIN.cpp (1)

• SharedLibrary (9-12)

include/behaviortree_cpp/loggers/bt_cout_logger.h (1)

src/loggers/bt_cout_logger.cpp (2)

• StdCoutLogger (6-7)
• StdCoutLogger (8-8)

include/behaviortree_cpp/loggers/bt_observer.h (1)

src/loggers/bt_observer.cpp (2)

• TreeObserver (7-44)
• TreeObserver (46-47)

include/behaviortree_cpp/loggers/bt_sqlite_logger.h (1)

src/loggers/bt_sqlite_logger.cpp (2)

• SqliteLogger (55-129)
• SqliteLogger (131-146)

include/behaviortree_cpp/action_node.h (1)

src/action_node.cpp (5)

• ActionNodeBase (21-23)
• SyncActionNode (53-55)
• SimpleActionNode (27-31)
• CoroActionNode (83-85)
• CoroActionNode (87-97)

include/behaviortree_cpp/actions/sleep_node.h (1)

src/actions/sleep_node.cpp (1)

• SleepNode (6-8)

include/behaviortree_cpp/decorator_node.h (1)

src/decorator_node.cpp (2)

• DecoratorNode (18-20)
• SimpleDecoratorNode (66-70)

include/behaviortree_cpp/controls/parallel_all_node.h (1)

src/controls/parallel_all_node.cpp (1)

• ParallelAllNode (21-23)

include/behaviortree_cpp/decorators/subtree_node.h (1)

src/decorators/subtree_node.cpp (1)

• SubTreeNode (3-7)

include/behaviortree_cpp/condition_node.h (1)

src/condition_node.cpp (2)

• ConditionNode (18-20)
• SimpleConditionNode (22-26)

include/behaviortree_cpp/decorators/delay_node.h (1)

src/decorators/delay_node.cpp (2)

• DelayNode (8-12)
• DelayNode (14-16)

include/behaviortree_cpp/decorators/repeat_node.h (1)

src/decorators/repeat_node.cpp (2)

• RepeatNode (19-26)
• RepeatNode (28-33)

include/behaviortree_cpp/loggers/bt_minitrace_logger.h (1)

src/loggers/bt_minitrace_logger.cpp (2)

• MinitraceLogger (10-16)
• MinitraceLogger (18-22)

src/xml_parsing.cpp (1)

include/behaviortree_cpp/xml_parsing.h (1)

• XMLParser (16-44)

include/behaviortree_cpp/blackboard.h (1)

src/blackboard.cpp (3)

• Entry (310-316)
• other (318-329)
• other (318-318)

include/behaviortree_cpp/control_node.h (1)

src/control_node.cpp (1)

• ControlNode (18-20)

include/behaviortree_cpp/controls/sequence_with_memory_node.h (1)

src/controls/sequence_with_memory_node.cpp (1)

• SequenceWithMemory (18-22)

include/behaviortree_cpp/decorators/inverter_node.h (1)

src/decorators/inverter_node.cpp (1)

• InverterNode (18-21)

include/behaviortree_cpp/controls/while_do_else_node.h (1)

src/controls/while_do_else_node.cpp (1)

• WhileDoElseNode (17-21)

🪛 Cppcheck (2.19.0)

src/xml_parsing.cpp

[error] 376-376: Found an exit path from function with non-void return type that has missing return statement

(missingReturn)

🪛 Shellcheck (0.11.0)

run_clang_tidy.sh

[warning] 63-63: Quote this to prevent word splitting.

(SC2046)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)

• GitHub Check: industrial_ci (rolling, main)
• GitHub Check: build (ubuntu-22.04, tsan)
• GitHub Check: build (ubuntu-22.04, asan_ubsan)
• GitHub Check: pixi_conda_build (windows-latest)
• GitHub Check: pixi_conda_build (ubuntu-latest)
• GitHub Check: build (ubuntu-22.04)
• GitHub Check: build (windows-latest)
• GitHub Check: industrial_ci (humble, main)
• GitHub Check: industrial_ci (jazzy, main)

🔇 Additional comments (54)

include/behaviortree_cpp/loggers/groot2_protocol.h (1)

142-143: LGTM! Good modernization.

The refactor correctly moves initialization from the constructor body to the member initializer list, which is the preferred modern C++ idiom. The behavior remains identical (zero-initialization of the array).

include/behaviortree_cpp/utils/demangle_util.h (1)

46-49: LGTM! Explicit move deletion follows the Rule of Five.

The class already had implicitly suppressed move operations due to the user-declared destructor and deleted copy operations. Explicitly deleting them improves clarity and satisfies clang-tidy's cppcoreguidelines-special-member-functions check.

include/behaviortree_cpp/actions/sleep_node.h (2)

25-28: Good practice: Explicitly deleted special member functions.

Making SleepNode non-copyable and non-movable is appropriate given that it manages timer resources (timer_, timer_id_) and has a non-trivial destructor that calls halt(). This follows the Rule of Five and prevents accidental misuse of a stateful action node.

---

43-43: Good practice: Explicit member initialization.

Initializing timer_id_ to 0 prevents potential undefined behavior and provides a clear sentinel value for "no active timer." This defensive initialization aligns with modern C++ best practices.

include/behaviortree_cpp/decorators/repeat_node.h (2)

42-42: LGTM: Redundant virtual keyword removed.

The virtual keyword is redundant when override is present, as override already implies the function is virtual. This cleanup aligns with modern C++ best practices.

---

44-47: The copy/move semantics deletions are properly implemented and consistent. The base class DecoratorNode has identical deleted copy/move operators (lines 16-19 of decorator_node.h), and all concrete decorator nodes in the codebase follow this pattern uniformly. No usage of RepeatNode copy or move operations exists in the codebase, confirming that deleting these semantics aligns with the tree's ownership model where nodes are owned by the tree itself rather than copied or moved by user code.

include/behaviortree_cpp/controls/switch_node.h (2)

55-55: LGTM: Cleaner destructor declaration.

Removing the explicit virtual keyword when override is present is correct—the destructor remains virtual through inheritance. This improves code clarity by avoiding redundancy.

---

57-60: LGTM: Deleted copy/move operations prevent misuse.

Explicitly deleting copy and move operations is best practice for behavior tree node types. SwitchNode instances are managed through the factory pattern and std::unique_ptr, ensuring tree integrity and preventing unintended ownership issues. This pattern is consistent across other control nodes in the codebase.

include/behaviortree_cpp/utils/timer_queue.h (2)

180-181: LGTM! Correctly prevents moving of active TimerQueue.

Deleting the move operations is necessary because the worker thread (started in the constructor at line 82) captures this and runs asynchronously. Moving the object would invalidate the pointer used by the running thread, leading to undefined behavior. This change aligns with the already-deleted copy operations and prevents unsafe usage.

---

258-258: LGTM! Good defensive initialization.

Adding the default initializer id = 0 ensures WorkItem starts in a safe "cancelled" state (consistent with the semantic meaning of id == 0 used throughout the code at lines 136, 168, and 244). This prevents potential uninitialized memory issues if WorkItem is ever default-constructed without explicit initialization.

include/behaviortree_cpp/loggers/bt_observer.h (1)

23-26: LGTM! Good design enforcement for observer semantics.

Making TreeObserver non-copyable and non-movable is appropriate for an observer class that maintains internal mappings to a specific tree and registers callbacks. The parent class StatusChangeLogger allows move semantics but deletes copies; TreeObserver further restricts by deleting moves as well, which is a valid design choice for stable observer identity. Current usage patterns in tests and examples confirm no code relies on copy or move semantics—all instances are constructed locally and used in place.

include/behaviortree_cpp/utils/shared_library.h (1)

130-133: LGTM! No breaking changes found.

The deleted copy and move operations are correctly declared and align with the class's resource-management responsibilities. The codebase only instantiates SharedLibrary as local variables (as seen in bt_factory.cpp), never copying or moving instances. This deletion does not affect any existing usage patterns.

src/xml_parsing.cpp (10)

20-20: LGTM: Clean addition of helper and include.

The tuple include supports std::ignore usage later in the file, and the StrEqual lambda in the anonymous namespace provides a clearer, more expressive alternative to raw strcmp calls.

Also applies to: 89-94

---

121-121: LGTM: Factory pointer conversion properly initialized.

Converting the factory member from a reference to a pointer with default initialization improves flexibility. The constructor correctly initializes it to the address of the factory parameter, ensuring a valid pointer throughout the object's lifetime.

Also applies to: 129-129

---

151-152: LGTM: Move constructor correctly marked noexcept.

Marking the move constructor as noexcept is a best practice that enables optimizations in standard containers and algorithms, particularly for move operations in vectors and other containers.

---

165-165: LGTM: Excellent modernization with std::make_unique.

Replacing raw new with std::make_unique improves exception safety, clarifies ownership semantics, and follows modern C++ best practices. This change is consistently applied across all XMLDocument allocations.

Also applies to: 177-177, 314-314

---

244-245: LGTM: Proper use of std::ignore for discarded values.

Using std::ignore explicitly documents the intent to discard return values and suppresses compiler warnings, which is cleaner than void casts or other alternatives.

Also applies to: 370-371, 378-379, 777-777

---

296-296: LGTM: NOLINT appropriately used.

The NOLINT(misc-const-correctness) is correctly applied here since the variable's const-correctness depends on conditional compilation paths. In the USING_ROS2 case, it's assigned from a function call, justifying the suppression.

---

220-220: LGTM: Explicit nullptr checks improve clarity.

Converting implicit pointer checks to explicit nullptr comparisons enhances code readability and aligns with modern C++ style guidelines. This change is consistently applied throughout the file.

Also applies to: 250-250, 256-256, 274-274, 287-287, 349-349, 396-396, 402-406, 411-411, 423-423, 446-446, 638-638, 646-646, 669-670, 679-679, 767-767, 880-880

---

882-882: LGTM: Pointer semantics with auto improves clarity.*

Using auto* with dynamic_cast explicitly communicates that the result is a pointer, making the code more self-documenting compared to plain auto. This is a good modern C++ practice.

Also applies to: 886-886, 1127-1127, 1171-1171, 1178-1178

---

632-632: LGTM: Using count() for broader C++ standard compatibility.

Replacing contains() with count() == 0 maintains compatibility with C++ standards prior to C++20, while providing the same functionality for set-like containers.

Also applies to: 1204-1204

---

1062-1063: LGTM: Anonymous namespace properly scopes helper functions.

Placing the helper functions addNodeModelToXML and addTreeToXML in an anonymous namespace provides internal linkage, preventing potential name collisions and clearly indicating these are file-local utilities.

Also applies to: 1215-1215

include/behaviortree_cpp/loggers/bt_sqlite_logger.h (1)

58-63: LGTM! Non-copyable/non-movable semantics are appropriate.

The deleted copy/move operations are appropriate for SqliteLogger since it manages resources like a database connection (sqlite3* db_) and a writer thread (std::thread writer_thread_). The destructor change removes the redundant virtual keyword while keeping override, which is a style improvement.

CMakeLists.txt (1)

309-322: LGTM! Helpful addition for IDE support and clang-tidy integration.

The generated .clangd configuration provides proper compile flags and include paths for the clangd language server, which improves developer experience and enables better static analysis. The configuration correctly matches the project's C++17 standard and includes all necessary paths.

include/behaviortree_cpp/controls/parallel_all_node.h (2)

1-1: Copyright update looks good.

Routine maintenance updating the copyright year range.

---

48-51: LGTM! Appropriate for control nodes managing child lifecycle.

Deleting copy/move operations is appropriate for ParallelAllNode since it manages child nodes and maintains execution state (completed_list_, failure_count_). This prevents accidental copying of complex control flow state.

src/controls/parallel_all_node.cpp (1)

1-1: Copyright update looks good.

Routine maintenance updating the copyright year range.

include/behaviortree_cpp/controls/parallel_node.h (2)

2-2: Copyright update looks good.

Routine maintenance updating the copyright year range.

---

59-62: LGTM! Appropriate for control nodes managing execution state.

Deleting copy/move operations is appropriate for ParallelNode since it manages child node execution and maintains state (completed_list_, success_count_, failure_count_).

include/behaviortree_cpp/controls/if_then_else_node.h (2)

1-1: Copyright update looks good.

Routine maintenance updating the copyright year range.

---

39-44: LGTM! Good defensive programming practices.

The destructor change removes the redundant virtual keyword (implied by override), and the deleted copy/move operations appropriately prevent accidental copying of control flow state.

include/behaviortree_cpp/loggers/bt_minitrace_logger.h (1)

12-17: LGTM! Appropriate for logger managing external resources.

The destructor change removes the redundant virtual keyword, and the deleted copy/move operations are appropriate since MinitraceLogger manages minitrace library resources (initialized via mtr_init and cleaned up via mtr_flush/mtr_shutdown).

include/behaviortree_cpp/loggers/bt_cout_logger.h (1)

20-23: LGTM! Consistent with other logger hardening.

Deleting copy/move operations is appropriate for StdCoutLogger to prevent accidental duplication of logger state and tree observation callbacks.

include/behaviortree_cpp/decorators/subtree_node.h (1)

57-62: LGTM!

The destructor style update (removing redundant virtual with override) and explicit deletion of copy/move operations align with the PR's goal of enforcing non-copyable/non-movable semantics across BT nodes. This is consistent with the broader pattern applied throughout the codebase.

include/behaviortree_cpp/controls/while_do_else_node.h (1)

38-43: LGTM!

Consistent with the repository-wide pattern of enforcing non-copyable/non-movable semantics for control nodes.

include/behaviortree_cpp/controls/sequence_node.h (1)

40-45: LGTM!

The changes consistently apply non-copyable/non-movable semantics to SequenceNode, matching the pattern across other control and decorator nodes in this PR.

include/behaviortree_cpp/decorators/script_precondition.h (1)

30-35: LGTM!

Explicit deletion of copy/move operations is appropriate for PreconditionNode given its stateful members (_executor, _children_running). This aligns with the PR-wide enforcement of non-copyable/non-movable semantics.

include/behaviortree_cpp/json_export.h (2)

54-64: LGTM!

The changes properly enforce the singleton pattern for JsonExporter by:

1. Making the default constructor private
2. Deleting copy/move operations
3. Preserving the public static get() accessor

This prevents unintended instantiation or duplication of the exporter.

---

256-276: LGTM!

The NOLINTBEGIN/NOLINTEND annotations appropriately suppress bugprone-macro-parentheses for this macro, which intentionally uses unparenthesized parameters as part of its design pattern for JSON converters.

include/behaviortree_cpp/condition_node.h (2)

26-31: LGTM!

Consistent application of non-copyable/non-movable semantics to the ConditionNode base class.

---

66-69: LGTM!

Deleting copy/move operations for SimpleConditionNode is appropriate given its TickFunctor member (which wraps a std::function). This prevents unintended sharing of callback state.

run_clang_tidy.sh (1)

66-72: No action needed on --check flag usage.

The --check flag has been available since clangd-12 and is not a new feature. The --check-locations=false flag disables exercising language features (hover, analyses) at every point in the file—a performance optimization when checking a single file—not a suppression of diagnostics. This flag combination is appropriate and recommended for this use case.

Likely an incorrect or invalid review comment.

include/behaviortree_cpp/action_node.h (4)

41-45: LGTM! Explicit non-copyable/non-movable semantics for ActionNodeBase.

The deletion of copy/move operations is appropriate for tree node types that have identity semantics and shouldn't be duplicated.

---

63-67: LGTM! Consistent pattern applied to SyncActionNode.

---

100-104: LGTM! SimpleActionNode correctly marked non-copyable/non-movable.

The class stores a std::function callback, and explicit deletion prevents accidental copies that could lead to unexpected behavior.

---

215-220: LGTM! CoroActionNode properly marked non-copyable/non-movable.

The destructor declaration change (line 215) and explicit deletions are correct. The class uses Pimpl idiom with std::unique_ptr, so explicit deletion reinforces the intended semantics.

include/behaviortree_cpp/decorators/delay_node.h (1)

46-50: LGTM! Explicit non-copyable/non-movable semantics for DelayNode.

The class manages timer resources (TimerQueue, timer_id_) and synchronization primitives, making explicit deletion of copy/move operations appropriate. The destructor properly calls halt() to clean up timer resources.

include/behaviortree_cpp/control_node.h (1)

29-34: LGTM! ControlNode properly updated with non-copyable/non-movable semantics.

The destructor change (removing redundant virtual since override implies virtuality) and explicit deletion of copy/move operations are correct. Control nodes manage child relationships and shouldn't be copied.

include/behaviortree_cpp/controls/sequence_with_memory_node.h (1)

39-44: LGTM! SequenceWithMemory correctly marked non-copyable/non-movable.

The class maintains execution state (current_child_idx_, skipped_count_) that would cause incorrect behavior if copied. The destructor change and explicit deletions are consistent with the base ControlNode class.

include/behaviortree_cpp/decorators/inverter_node.h (1)

30-35: LGTM! InverterNode correctly marked non-copyable/non-movable.

Consistent with the base DecoratorNode class and the broader PR pattern for tree node types.

include/behaviortree_cpp/decorator_node.h (2)

16-21: LGTM! DecoratorNode base class properly marked non-copyable/non-movable.

The destructor change (removing redundant virtual) and explicit deletion of copy/move operations are correct. As a base class for all decorators, this establishes the non-copyable/non-movable pattern for the entire decorator hierarchy.

---

68-71: LGTM! SimpleDecoratorNode correctly marked non-copyable/non-movable.

Consistent with the base DecoratorNode class. The class stores a TickFunctor callback, and explicit deletion prevents accidental copies that could lead to unexpected behavior.

include/behaviortree_cpp/blackboard.h (2)

43-46: LGTM: Non-copyable and non-movable semantics are appropriate.

Deleting copy and move operations is the correct design choice for Blackboard, which is intended to be used exclusively via std::shared_ptr (as evidenced by the protected constructor and static create() factory method). This prevents accidental copying and enforces shared ownership semantics.

---

62-66: Entry special member functions correctly omit the mutex member.

The custom copy operations and deleted move operations are necessary because Entry contains a mutable std::mutex entry_mutex which is neither copyable nor movable. The implementation in src/blackboard.cpp correctly omits the mutex from copy operations (default-constructing it instead) and includes a self-assignment check.

Entry objects are never copied directly in the codebase; they are accessed exclusively through shared_ptr<Entry> pointers. The primary use case for copying Entry fields is in cloneInto(), which copies field values individually while holding locks on both the source and destination Blackboard storage mutexes—not using the Entry copy operations at all. Thread safety is properly maintained through Blackboard-level locking combined with per-Entry value access protection via entry_mutex.

Likely an incorrect or invalid review comment.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

include/behaviortree_cpp/loggers/abstract_logger.h (1)

47-50: Comment contradicts initialization value — pre-existing documentation issue.

The comment on line 47 states "false by default," but show_transition_to_idle_ has been initialized to true throughout the repository history. This means IDLE transitions are logged by default (see line 76), contrary to what the comment documents.

This is a pre-existing inconsistency unrelated to the refactor. Either update the comment to "true by default" or verify the intended behavior and adjust the initialization accordingly.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between db1b06f and 1b40113.

📒 Files selected for processing (9)

• include/behaviortree_cpp/basic_types.h
• include/behaviortree_cpp/bt_factory.h
• include/behaviortree_cpp/controls/switch_node.h
• include/behaviortree_cpp/decorators/script_precondition.h
• include/behaviortree_cpp/loggers/abstract_logger.h
• include/behaviortree_cpp/loggers/groot2_protocol.h
• include/behaviortree_cpp/tree_node.h
• include/behaviortree_cpp/utils/demangle_util.h
• include/behaviortree_cpp/utils/timer_queue.h

✅ Files skipped from review due to trivial changes (1)

• include/behaviortree_cpp/basic_types.h

🚧 Files skipped from review as they are similar to previous changes (1)

• include/behaviortree_cpp/tree_node.h

🧰 Additional context used

🧬 Code graph analysis (2)

include/behaviortree_cpp/bt_factory.h (3)

sample_nodes/crossdoor_nodes.cpp (1)

• BT_REGISTER_NODES (80-84)

sample_nodes/dummy_nodes.cpp (1)

• BT_REGISTER_NODES (5-8)

sample_nodes/movebase_node.cpp (1)

• BT_REGISTER_NODES (6-9)

include/behaviortree_cpp/decorators/script_precondition.h (1)

include/behaviortree_cpp/basic_types.h (1)

• NodeStatus (33-230)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)

• GitHub Check: pixi_conda_build (ubuntu-latest)
• GitHub Check: pixi_conda_build (windows-latest)
• GitHub Check: build (ubuntu-22.04, tsan)
• GitHub Check: build (ubuntu-22.04)
• GitHub Check: build (windows-latest)
• GitHub Check: industrial_ci (rolling, main)
• GitHub Check: build (ubuntu-22.04, asan_ubsan)
• GitHub Check: industrial_ci (jazzy, main)
• GitHub Check: industrial_ci (humble, main)

🔇 Additional comments (20)

include/behaviortree_cpp/loggers/groot2_protocol.h (3)

135-135: LGTM! Good modernization.

Using default member initialization for tree_id is a best practice that simplifies the struct and eliminates the need for an explicit constructor. The = {} syntax correctly zero-initializes all 16 bytes of the array.

---

150-155: LGTM! Cleaner code.

Removing the unnecessary reinterpret_cast<char*> simplifies the code since memcpy accepts void* arguments. This change makes the code more idiomatic and easier to read.

---

184-184: LGTM! Good defensive programming.

Explicitly initializing type to 0 before deserialization ensures the variable is in a known state, even if deserialization fails. This is a clang-tidy best practice that improves code safety.

include/behaviortree_cpp/utils/demangle_util.h (3)

48-49: LGTM! Proper resource management.

Deleting the move operations is correct. The class manages a resource via demangle_alloc/demangle_free without implementing move semantics, so preventing moves avoids resource management bugs.

---

63-63: LGTM! Appropriate linter suppression.

The NOLINTNEXTLINE is correct. The code interfaces with __cxa_demangle, which allocates via malloc, so std::free is unavoidable.

---

109-113: LGTM! Improved readability.

The explicit nullptr check and simplified control flow (removing the else branch) make the code clearer without changing behavior.

include/behaviortree_cpp/bt_factory.h (2)

2-2: LGTM! Copyright year updated.

Standard maintenance to reflect current year.

---

81-84: LGTM! Appropriate NOLINT directives for plugin macro.

The suppressions are well-justified for this legitimate macro use case:

• cppcoreguidelines-macro-usage: The macro abstracts cross-platform plugin export (extern "C" + visibility attributes) and provides a clean public API that cannot be easily replaced with inline functions or constexpr.
• bugprone-macro-parentheses: The factory parameter is used as a typed function parameter (BT::BehaviorTreeFactory& factory), not as an expression requiring parentheses.

The directives are properly scoped with BEGIN/END markers.

include/behaviortree_cpp/controls/switch_node.h (4)

1-1: LGTM!

Copyright year update is appropriate.

---

55-60: LGTM!

Good improvements:

• Removing redundant virtual when override is present aligns with modernize-use-override.
• Explicitly deleting copy and move operations follows the Rule of Five and is appropriate for tree-managed nodes that maintain runtime state (running_child_).

---

67-67: LGTM!

Using in-class default member initialization ensures running_child_ is consistently initialized, even if additional constructors are added later.

---

78-78: LGTM!

Constructor initializer list correctly simplified after moving running_child_ initialization to the member declaration.

include/behaviortree_cpp/decorators/script_precondition.h (3)

30-30: LGTM: Redundant virtual keyword removed.

The virtual keyword is redundant when override is specified, as the function is already virtual through inheritance from DecoratorNode. This cleanup improves code clarity.

---

32-35: LGTM: Explicit non-copyable/non-movable semantics.

Explicitly deleting copy and move operations clarifies that behavior tree nodes have unique ownership semantics and should not be duplicated or transferred. This defensive pattern prevents subtle bugs and aligns with the behavior tree lifecycle model.

---

50-50: LGTM: Defensive initialization added.

Initializing else_return to NodeStatus::FAILURE (matching the port's default value) is good defensive programming. While getInput should always succeed due to the default port value, explicit initialization prevents potential use-before-initialization issues if the control flow is modified in future refactorings.

include/behaviortree_cpp/loggers/abstract_logger.h (1)

59-62: Modern initialization pattern is a good practice.

Using default member initializers is a clean, modern C++ approach that ensures these members are always initialized consistently. This refactor improves code clarity by making default values visible at the declaration site.

Note: Ensure the value for show_transition_to_idle_ on line 60 matches the intended default behavior (see the previous comment regarding the inconsistency with the comment on line 47).

include/behaviortree_cpp/utils/timer_queue.h (4)

74-75: Good naming convention fix.

Renaming from _Clock/_Duration to ClockT/DurationT avoids identifiers that start with underscore followed by uppercase, which are reserved by the C++ standard for implementation use.

---

178-181: LGTM: Completing Rule of Five by deleting move operations.

The class manages a running thread and synchronization primitives (std::mutex, atomics), making it non-trivially movable. Explicitly deleting move operations is the correct defensive choice here.

---

255-264: Good addition of explicit default initialization.

The id = 0 default initialization (line 258) ensures the member is always initialized, preventing potential undefined behavior from uninitialized reads.

---

99-114: Consistent template parameter usage throughout.

All time-point calculations and type references have been correctly updated to use ClockT and DurationT. The logic remains unchanged - default-constructed time_point (epoch) correctly represents "immediate execution" for cancelled timers.

Also applies to: 121-153, 158-176, 184-209, 211-231, 233-248

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

.github/workflows/pre-commit.yaml (1)

23-28: Consider verifying the install script or using alternative installation methods.

Downloading and executing scripts with sudo without checksum verification poses a security risk. While apt.llvm.org is the official LLVM repository and this pattern is common, consider adding checksum verification or using GitHub's setup actions.

💡 Alternative approaches

Option 1: Verify script checksum before execution

     - name: Install LLVM 21
       run: |
         wget https://apt.llvm.org/llvm.sh
+        echo "expected_checksum llvm.sh" | sha256sum -c
         chmod +x llvm.sh
         sudo ./llvm.sh 21
         sudo apt-get install -y clangd-21 clang-tidy-21

Option 2: Use existing GitHub action

     - name: Install LLVM 21
       uses: KyleMayes/install-llvm-action@v2
       with:
         version: "21"

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1b40113 and 5f890c9.

📒 Files selected for processing (1)

• .github/workflows/pre-commit.yaml

🧰 Additional context used

🪛 actionlint (1.7.9)

.github/workflows/pre-commit.yaml

21-21: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)

• GitHub Check: build (windows-latest)
• GitHub Check: pixi_conda_build (windows-latest)
• GitHub Check: pixi_conda_build (ubuntu-latest)
• GitHub Check: industrial_ci (jazzy, main)
• GitHub Check: industrial_ci (rolling, main)
• GitHub Check: build (ubuntu-22.04, tsan)
• GitHub Check: build (ubuntu-22.04, asan_ubsan)
• GitHub Check: industrial_ci (humble, main)
• GitHub Check: build (ubuntu-22.04)

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

.github/workflows/pre-commit.yaml (1)

14-14: Update deprecated action to prevent CI failures.

Both the pre-commit and clang-tidy jobs use actions/checkout@v3, which targets the deprecated Node.js 16 runner and will eventually stop working.

🔎 Proposed fix

-    - uses: actions/checkout@v3
+    - uses: actions/checkout@v4

Apply this change to both line 14 and line 21.

Also applies to: 21-21

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5f890c9 and cbac2ce.

📒 Files selected for processing (1)

• .github/workflows/pre-commit.yaml

🧰 Additional context used

🪛 actionlint (1.7.9)

.github/workflows/pre-commit.yaml

21-21: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)

• GitHub Check: industrial_ci (jazzy, main)
• GitHub Check: industrial_ci (humble, main)
• GitHub Check: industrial_ci (rolling, main)
• GitHub Check: build (ubuntu-22.04)
• GitHub Check: pixi_conda_build (windows-latest)
• GitHub Check: build (windows-latest)
• GitHub Check: pixi_conda_build (ubuntu-latest)
• GitHub Check: build (ubuntu-22.04, tsan)
• GitHub Check: build (ubuntu-22.04, asan_ubsan)

🔇 Additional comments (2)

.github/workflows/pre-commit.yaml (2)

23-28: LLVM and clang-tidy installation looks good.

The installation of clangd-21 and clang-tidy-21 addresses the concern from the previous review about missing dependencies. The approach using the official llvm.sh script is appropriate.

---

38-39: No action needed. The script run_clang_tidy.sh is already committed with executable permissions (755), so the workflow will execute correctly without additional chmod steps.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (4)

src/blackboard.cpp (1)

174-176: Consider using structured bindings with just the key.

Using std::ignore works but is slightly verbose. You could use a single-element binding since only the key is needed.

🔎 Alternative using a simpler binding

-  for(const auto& [key, entry] : dst_storage)
-  {
-    std::ignore = entry;  // unused in this loop
+  for(const auto& [key, _] : dst_storage)
+  {

Note: Using _ as a placeholder is supported in C++26 but may generate warnings on older standards. The current approach with std::ignore is safer for broader compatibility.

run_clang_tidy.sh (2)

3-4: Consider using dirname for clarity.

The parameter expansion ${0%/*} works correctly but dirname "$0" is more readable and conventional for extracting the directory path.

🔎 Proposed refactor

-script_dir=${0%/*}
+script_dir=$(dirname "$0")
 ws_dir=$(realpath "$script_dir")

---

32-32: Remove or use the clang_tidy_paths variable consistently.

The variable clang_tidy_paths is assigned but never used in the actual find command at line 62, which directly specifies "$ws_dir/src" "$ws_dir/include" instead. This creates maintenance confusion.

🔎 Proposed refactor

Either remove the unused variable:

-clang_tidy_paths="$ws_dir/src $ws_dir/include"
 cmake_build_path="$ws_dir/${1:-build}"

Or use an array and reference it consistently:

-clang_tidy_paths="$ws_dir/src $ws_dir/include"
+clang_tidy_paths=("$ws_dir/src" "$ws_dir/include")
 cmake_build_path="$ws_dir/${1:-build}"
...
-find "$ws_dir/src" "$ws_dir/include" \( -name '*.cpp' -o -name '*.hpp' -o -name '*.h' \) -not -name '*_WIN.cpp' "${skip_paths[@]}" -print0 \
+find "${clang_tidy_paths[@]}" \( -name '*.cpp' -o -name '*.hpp' -o -name '*.h' \) -not -name '*_WIN.cpp' "${skip_paths[@]}" -print0 \

include/behaviortree_cpp/utils/simple_string.hpp (1)

36-44: Consider copy-and-swap idiom for exception safety.

While the current implementation is functionally safe (if createImpl throws, the object remains in a valid empty state), explicitly calling the destructor and continuing to use the object is unconventional and error-prone. The copy-and-swap idiom is more idiomatic and clearly exception-safe.

🔎 Recommended refactor using copy-and-swap

  SimpleString& operator=(const SimpleString& other)
  {
    if(this != &other)
    {
-     this->~SimpleString();
-     createImpl(other.data(), other.size());
+     SimpleString temp(other);
+     std::swap(_storage, temp._storage);
    }
    return *this;
  }

This allows the temporary to clean up the old resources via its destructor, making the exception safety explicit.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cbac2ce and 0f37983.

📒 Files selected for processing (9)

• .github/dependabot.yml
• .github/workflows/pre-commit.yaml
• include/behaviortree_cpp/blackboard.h
• include/behaviortree_cpp/utils/simple_string.hpp
• run_clang_tidy.sh
• src/basic_types.cpp
• src/blackboard.cpp
• src/controls/reactive_fallback.cpp
• src/xml_parsing.cpp

✅ Files skipped from review due to trivial changes (1)

• .github/dependabot.yml

🚧 Files skipped from review as they are similar to previous changes (2)

• .github/workflows/pre-commit.yaml
• src/controls/reactive_fallback.cpp

🧰 Additional context used

🧬 Code graph analysis (1)

src/xml_parsing.cpp (1)

include/behaviortree_cpp/xml_parsing.h (1)

• XMLParser (16-44)

🪛 Cppcheck (2.19.0)

src/xml_parsing.cpp

[error] 376-376: Found an exit path from function with non-void return type that has missing return statement

(missingReturn)

🪛 Shellcheck (0.11.0)

run_clang_tidy.sh

[warning] 63-63: Quote this to prevent word splitting.

(SC2046)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

• GitHub Check: industrial_ci (rolling, main)
• GitHub Check: build (ubuntu-22.04, tsan)
• GitHub Check: build (ubuntu-22.04)
• GitHub Check: build (ubuntu-22.04, asan_ubsan)
• GitHub Check: pixi_conda_build (ubuntu-latest)
• GitHub Check: build (windows-latest)
• GitHub Check: clang-tidy
• GitHub Check: industrial_ci (jazzy, main)
• GitHub Check: industrial_ci (humble, main)
• GitHub Check: pixi_conda_build (windows-latest)

🔇 Additional comments (38)

include/behaviortree_cpp/blackboard.h (2)

43-46: LGTM! Non-copyable/non-movable semantics are appropriate for Blackboard.

Since Blackboard is designed to be shared via std::shared_ptr<Blackboard> (the Ptr alias) and created through the static factory create(), explicitly deleting copy/move operations prevents accidental slicing or unintended duplication of the shared state.

---

62-66: LGTM! Explicit deletion clarifies intent.

Entry was already implicitly non-copyable/non-movable due to the std::mutex entry_mutex member. Explicitly deleting these operations documents the design intent and prevents future confusion if the mutex were ever removed.

src/blackboard.cpp (8)

2-2: LGTM!

The <tuple> header is required for std::ignore usage introduced in cloneInto().

---

9-15: LGTM! Anonymous namespace provides internal linkage.

Wrapping IsPrivateKey in an anonymous namespace is a modern C++ best practice for file-local functions, equivalent to static but preferred in C++.

---

47-48: LGTM!

The NOLINTNEXTLINE comment appropriately suppresses the clang-tidy warning for this intentional const_cast. The cast is a valid pattern here to avoid duplicating the implementation of the const overload.

---

61-61: LGTM!

Making the unique_lock const enforces that it won't be unlocked or released prematurely, which is appropriate for simple scoped locking.

---

139-139: LGTM!

Consistent with the const-lock pattern used elsewhere in this file.

---

224-224: LGTM!

Consistent const-lock pattern for scoped mutex acquisition.

---

281-281: LGTM!

Making name const is a good const-correctness improvement since it's not modified after initialization.

---

313-314: LGTM!

The NOLINTNEXTLINE appropriately suppresses the warning for this standard const/non-const overload pattern where the non-const version delegates to the const version and casts away constness.

run_clang_tidy.sh (6)

6-19: LGTM!

The clangd-21 availability check is properly implemented using command -v, and the installation instructions are helpful and accurate.

---

35-45: LGTM!

The skip list setup is well-structured with proper array handling and quoting. The conversion to find exclusion patterns is correct.

---

47-52: LGTM!

The compile_commands.json existence check is properly implemented with a helpful error message guiding users on resolution.

---

54-60: LGTM!

The informational banner is helpful for users to understand what paths are being checked and skipped.

---

72-72: Verify sed substitution handles special characters in filenames.

The sed command sed "s/^/${1//\//\\/}: /" attempts to escape forward slashes in the filename for use in the sed substitution pattern. However, if filenames contain other sed special characters (e.g., &, \, [, ]), the substitution could break or produce incorrect output.

Consider testing with filenames containing special characters, or use a more robust approach such as:

# Alternative: use awk or printf instead of sed for prefixing
2>&1 | awk -v prefix="$1: " '{print prefix $0}'

This avoids sed metacharacter issues entirely.

---

75-77: LGTM!

The completion banner provides clear feedback that the process has finished.

src/basic_types.cpp (6)

50-72: Color code comments are now correct.

The previous review flagged swapped comments for ANSI codes. This has been fixed:

• \x1b[32m is correctly commented as GREEN (line 55)
• \x1b[31m is correctly commented as RED (line 59)

---

121-122: LGTM!

Using std::ignore to suppress the unused ptr from std::from_chars is an idiomatic approach. The error handling via ec is preserved.

---

143-156: LGTM!

The ConvertWithBoundCheck helper cleanly extracts common bounds-checking logic. Using int64_t as the intermediate type works correctly for all target types (int8 through uint32), as they all fit within its representable range.

---

200-217: LGTM!

The locale handling pattern is correct: save the current locale, set to "C" for consistent parsing, parse the value, then restore the original locale. The use of std::ignore to silence the unused return from setlocale is appropriate.

---

446-450: Good fix for potential UB.

Casting to unsigned char before calling std::isalpha is the correct approach. Without this cast, a signed char with a negative value (extended ASCII) would invoke undefined behavior.

---

494-508: LGTM!

The rewritten StartWith function is safer for StringView usage. It properly handles bounds checking first (line 496) and performs explicit character comparison, avoiding assumptions about null-termination that strncmp would require.

src/xml_parsing.cpp (9)

89-94: LGTM!

The StrEqual helper lambda in the anonymous namespace is a clean way to encapsulate the strcmp comparison logic.

---

121-130: LGTM!

The change from reference to pointer for factory with explicit initialization enables proper move semantics. The suffix_count initialization to 0 is good for clarity. The constructor correctly takes the address of the factory reference.

---

165-167: LGTM!

Using std::make_unique<XMLDocument>() is the preferred approach for exception safety and cleaner code compared to raw new.

---

376-381: Static analysis false positive - no action needed.

The Cppcheck warning about a missing return statement is a false positive. The ThrowError lambda always exits via throw RuntimeError(buffer), which is a valid exit path. Since the lambda never returns normally, no return statement is required.

---

693-697: Typo has been fixed.

The variable is now correctly named is_blackboard (previously is_blacbkboard as flagged in a prior review).

---

220-223: LGTM!

The consistent use of explicit == nullptr comparisons throughout the file improves readability and aligns with modern C++ style guidelines.

---

244-246: LGTM!

Using std::ignore to suppress the unused return value from snprintf is appropriate here. The buffer is fixed-size and the context (error message formatting) tolerates truncation if it were to occur.

---

882-890: LGTM!

The dynamic_cast usage with auto* is clean and the null checks are correctly performed before dereferencing.

---

775-778: LGTM!

Using std::ignore to explicitly indicate that remap_value is intentionally unused in this loop is a good practice for clarity and silencing warnings.

include/behaviortree_cpp/utils/simple_string.hpp (7)

21-25: LGTM! Default constructor correctly initializes empty SOO state.

The default constructor properly initializes an empty small-string-optimized string by setting capacity_left = CAPACITY and null-terminating the buffer.

---

46-58: Excellent! Move semantics correctly fixed.

The previous issue with calling the destructor before swap in move assignment has been properly addressed. The current implementation correctly uses only std::swap, allowing the moved-from object to naturally clean up the original resources. Both move constructor and move assignment are now noexcept and follow best practices.

---

112-130: Good refactoring of relational operators.

The refactoring of operator<= and operator>= to delegate to the existing < and > implementations follows the DRY principle and improves maintainability.

---

132-152: LGTM! Lexicographic comparison correctly implemented.

The refactored operator< and operator> correctly implement lexicographic ordering using memcmp for byte-wise comparison up to the minimum length, with size as the tie-breaker. This is the standard approach for string comparison.

---

154-157: LGTM! isSOO() correctly identifies storage mode.

The implementation correctly checks whether the IS_LONG_BIT is unset to determine SOO (small-string-optimized) mode, consistent with how createImpl sets this bit for heap-allocated strings.

---

162-164: Good practice: explicit default member initializers.

Adding default member initializers (data = nullptr, size = 0, capacity_left = CAPACITY, _storage = {}) makes the initialization semantics explicit and improves code clarity.

Also applies to: 178-180

---

194-194: LGTM! Appropriate use of NOLINT for intentional memory management.

The NOLINT annotation is justified here since SimpleString intentionally manages raw memory with explicit ownership semantics (allocation here, deallocation in the destructor).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Fix inconsistency between help message and actual argument handling.

The help message at line 23 documents two arguments [source_path] [build_path], but the script only uses one argument at line 33 (${1:-build}). The source paths are hardcoded at line 32, not configurable via arguments.

🔎 Proposed fix

-  echo "Usage: $(basename "$0") [source_path] [build_path]"
+  echo "Usage: $(basename "$0") [build_path]"
   echo "Run clang-tidy on the specified paths."
   echo
   echo "Arguments:"

🤖 Prompt for AI Agents

In run_clang_tidy.sh around lines 21-29, the help text mentions two positional
arguments ([source_path] [build_path]) but the script only reads one argument
and uses hardcoded source paths; update the script to accept and document both
positional args: set source_path="${1:-.}" and build_path="${2:-build}", update
the usage string to reflect defaults, replace any hardcoded source-path lists
with references to "$source_path" (or derive file lists from that directory),
and continue to use "$build_path" for compile_commands.json lookups so the help
and actual behavior are consistent.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Quote $(nproc) to prevent word splitting.

Shellcheck SC2046 warns that $(nproc) should be quoted. While xargs -P expects a numeric value and will typically work, quoting prevents potential issues if nproc returns unexpected output or fails.

🔎 Proposed fix

-  | xargs -0 -n 1 -P $(nproc) bash -c '
+  | xargs -0 -n 1 -P "$(nproc)" bash -c '

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	find "$ws_dir/src" "$ws_dir/include" \( -name '*.cpp' -o -name '*.hpp' -o -name '*.h' \) -not -name '*_WIN.cpp' "${skip_paths[@]}" -print0 \
	| xargs -0 -n 1 -P $(nproc) bash -c '
	set -o pipefail
	echo "$@"
	cd "'"$ws_dir"'" && clangd-21 \
	--log=error \
	--clang-tidy \
	--compile-commands-dir="'"$cmake_build_path"'" \
	--check-locations=false \
	--check="$@" \
	2>&1 | sed "s/^/${1//\//\\/}: /"
	' _
	find "$ws_dir/src" "$ws_dir/include" \( -name '*.cpp' -o -name '*.hpp' -o -name '*.h' \) -not -name '*_WIN.cpp' "${skip_paths[@]}" -print0 \
	| xargs -0 -n 1 -P "$(nproc)" bash -c '
	set -o pipefail
	echo "$@"
	cd "'"$ws_dir"'" && clangd-21 \
	--log=error \
	--clang-tidy \
	--compile-commands-dir="'"$cmake_build_path"'" \
	--check-locations=false \
	--check="$@" \
	2>&1 | sed "s/^/${1//\//\\/}: /"
	' _

🧰 Tools

🪛 Shellcheck (0.11.0)

[warning] 63-63: Quote this to prevent word splitting.

(SC2046)

🤖 Prompt for AI Agents

In run_clang_tidy.sh around lines 62 to 73, the xargs -P uses an unquoted
command substitution $(nproc); update it to quote the substitution to prevent
word splitting and unexpected tokenization by changing xargs -P $(nproc) to
xargs -P "$(nproc)". Ensure you only add the quotes around the substitution and
leave the rest of the command intact.