Skip to content

Conversation

@facontidavide
Copy link
Collaborator

@facontidavide facontidavide commented Jan 30, 2026

…ipt parser

The scripting engine evaluated "A - B + C" as "A - (B + C)" instead of "(A - B) + C". The root cause was that string_concat's operand chain referenced math_sum, causing it to appear twice in lexy's operation list with conflicting binding power levels. Changed string_concat::operand from math_sum to math_prefix to eliminate the duplicate entry.

Summary by CodeRabbit

  • Bug Fixes

    • Corrected operator associativity and string-vs-number comparison to prefer numeric semantics when possible.
  • Improvements

    • XML/blackboard now upgrade untyped entries to typed values when appropriate.
    • Loop node accepts vector/string inputs and can output dequeued values.
    • Enum parsing now accepts numeric forms; debug output shows remapped entry types.
    • Vector parsing supports a "json:" prefix for JSON-encoded vectors.
  • Tests

    • Added extensive parser, port, blackboard, enum, and regression tests; including large-error and negative-number cases.
  • Documentation

    • Added a consolidated bug tracker and workflow document.

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

facontidavide and others added 3 commits January 30, 2026 19:06
…ipt parser

The scripting engine evaluated "A - B + C" as "A - (B + C)" instead of
"(A - B) + C". The root cause was that string_concat's operand chain
referenced math_sum, causing it to appear twice in lexy's operation list
with conflicting binding power levels. Changed string_concat::operand
from math_sum to math_prefix to eliminate the duplicate entry.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…lid scripts

The bug (fixed in cb6c751) was that ValidateScript used a fixed char[2048]
buffer for error messages, causing out-of-bounds reads when error output
exceeded the buffer. The current code uses std::string with
std::back_inserter. This adds a regression test using the original
reproducer to ensure the fix holds.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Comparing against negative literals (e.g., "A!=-1") used to throw
"operator cannot be mixed with previous operators" because the unary
minus prefix was in a conflicting operator group. This was resolved by
the #1029 fix (string_concat operand change). This commit adds a
regression test covering both direct script evaluation and XML
precondition usage.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@coderabbitai
Copy link

coderabbitai bot commented Jan 30, 2026

📝 Walkthrough

Walkthrough

Adjusted scripting operator precedence for string concatenation; added and duplicated parser/unit tests; upgraded untyped blackboard string entries to typed values during XML parsing; extended LoopNode to accept/convert std::vector and string queues and expose dequeued output; added JSON vector parsing; multiple new port/node tests; added FIXME issue tracker.

Changes

Cohort / File(s) Summary
Grammar / Operators
include/behaviortree_cpp/scripting/operators.hpp
Added <cstdlib> include; changed Expression::string_concat::operand from math_sum to math_prefix; when both operands are strings, attempt numeric parse for numeric comparison before falling back to string compare; added explanatory comments.
Script Parser Tests
tests/script_parser_test.cpp
Added multiple regression tests for operator associativity, negative comparisons, and large-script validation; note: test blocks were duplicated within the file.
XML → Blackboard Upgrade
src/xml_parsing.cpp
When creating nodes, if an existing blackboard entry is an untyped string and the port is strongly typed, parse the string via the port and upgrade the entry's type and value instead of rejecting.
LoopNode / Ports
include/behaviortree_cpp/decorators/loop_node.h
Made queue port untyped to accept SharedQueue<T> or std::vector<T> (lazy conversion); accept string->queue via parsing; convert incoming std::vector<T> to internal deque on first use; added OutputPort<T>("value") and included <vector>.
Port/Action Tests
tests/gtest_ports.cpp
Added CollectDoubleAction, ActionWithDefaultEmptyVector, ProduceVectorDoubleAction and tests for SubTree→LoopDouble flows, default-empty-vector behavior, vector acceptance by Loop node, and getInput defaults.
Basic Types — vector parsing
src/basic_types.cpp
Added json:-prefixed parsing for convertFromString specializations of std::vector<T> to parse JSON arrays; preserved legacy semicolon-delimited parsing when json: not present.
Enum parsing / TreeNode
include/behaviortree_cpp/tree_node.h
Added numeric parsing path for enums using std::from_chars when scripting registry mapping is absent; included <charconv>.
getLockedPortContent behavior
src/tree_node.cpp
Lazily create a blackboard entry from manifest port type when a requested key is missing, then re-query and return the created entry.
Blackboard debug output
src/blackboard.cpp
Append parent entry type (demangled) for internal_to_external_ remappings in debugMessage output.
Blackboard Tests
tests/gtest_blackboard.cpp
Added ActionWithLockedPort, tests for locked-port default creation, numeric comparisons of string-stored numbers, and debug message remapping output verification.
Enums Tests
tests/gtest_enums.cpp
Added ActionWithNodeType and test validating enum parsing fallback via convertFromString<T> without registry.
Documentation / Tracker
FIXME.md
Added aggregated issue tracker with reproduction-driven workflow, categorized bug entries, and a summary table.
Misc / Includes
src/basic_types.cpp, other headers
Small include additions (<vector>, <charconv>, <cstdlib>) and related comments across affected files.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I nibbled code and changed a rule,
Concat now listens to prefix school,
Tests in pairs hop, double-tracked,
Blackboard learns and types get backed,
Vectors queue, JSON treats me cool. 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.87% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly identifies the main fix (left-associativity for arithmetic operators) and references issue #1029.
Description check ✅ Passed The description explains the root cause and the specific fix applied, addressing the main issue. However, it contains template comments that should be removed.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/github-reported-bugs

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

facontidavide and others added 3 commits January 30, 2026 19:57
…g remapping

When a SubTree passes a constant string literal (e.g., queue="1;2;3") to a
child tree's port, the subtree remapping stores it as a plain std::string in
the child blackboard without type information. Later, when a strongly-typed
node like LoopDouble reads the entry, Any::cast<SharedQueue<double>>() fails
because there's no conversion path from string to the expected type.

Fix: in createNodeFromXML, when initializing port entries, detect when an
existing blackboard entry is weakly typed but the port manifest declares a
strong type. In this case, convert the string value using the port's
parseString() method and upgrade the entry's type info in place.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ions

When a port of type std::vector<T> has a default empty value {} and no
input is specified in XML, toStr() converts the empty vector to "json:[]".
The explicit convertFromString specializations for vector types did not
handle the "json:" prefix (unlike the generic template), so this string
was split by ';' producing a vector with one literal "json:[]" element.

Fix: add "json:" prefix detection at the top of all four vector
convertFromString specializations (int, double, bool, string). When
detected, parse directly with nlohmann::json instead of splitting by ';'.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
LoopNode's queue port now uses an untyped BidirectionalPort to avoid
port type mismatch when upstream nodes produce std::vector<T>. At
runtime, tick() tries SharedQueue<T> first, then falls back to
std::vector<T> (converted to deque on first read), then string
(via convertFromString). This allows upstream nodes that output
vector<T> to feed directly into LoopNode without manual conversion.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

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/decorators/loop_node.h (1)

59-109: ⚠️ Potential issue | 🟠 Major

Reset dynamic queues on IDLE to avoid stale vector/string inputs.

For vector/string sources, current_queue_ stays non-null after the first run, so the !current_queue_ guard prevents re-reading updated port values on subsequent executions. Reset the queue when the node re-enters IDLE (and the port isn’t static) so each run can refresh from the port.

🐛 Suggested fix
     if(status() == NodeStatus::IDLE)
     {
       child_running_ = false;
       // special case: the port contains a string that was converted to SharedQueue<T>
       if(static_queue_)
       {
         current_queue_ = std::make_shared<std::deque<T>>();
         *current_queue_ = *static_queue_;
       }
+      else
+      {
+        current_queue_.reset();
+      }
     }
🤖 Fix all issues with AI agents
In `@FIXME.md`:
- Around line 12-23: Wrap all bare URLs in FIXME.md for the listed entries
(`#1029` and `#923`) to satisfy markdownlint MD034 by enclosing them in angle
brackets or using markdown link syntax; update both occurrences shown (the URLs
for the GitHub issues) so lines like - **URL:** https://github.com/... become -
**URL:** <https://github.com/...> (or [text](https://...)) and apply the same
wrapping consistently for any other bare URLs in the file.
- Around line 244-258: The markdown table separator row lacks spaces around the
pipe characters; update the separator row beneath the header "| Category | Count
| Issue Numbers |" so each pipe has a space on both sides (e.g., change
"|----------|-------|---------------|" to a separator with spaces around the
pipes and hyphens for each column) to satisfy MD060 table style.

Comment on lines +12 to +23
### [x] #1029 - Mathematical order of operations in script is wrong
- **URL:** https://github.com/BehaviorTree/BehaviorTree.CPP/issues/1029
- **Summary:** `A - B + C` is evaluated as `A - (B + C)` instead of left-to-right. The expression parser applies incorrect associativity for addition and subtraction at the same precedence level.
- **Component:** `src/scripting/`
- **Test file:** `tests/gtest_scripting.cpp` (or new)

### [x] #923 - Out-of-bounds read in script validation
- **URL:** https://github.com/BehaviorTree/BehaviorTree.CPP/issues/923
- **Summary:** `ValidateScript` can create a `std::string` from a non-NULL-terminated fixed-size stack buffer when parsing invalid scripts that generate large error messages, causing `strlen` to read out of bounds.
- **Component:** `src/scripting/`
- **Test file:** `tests/gtest_scripting.cpp` (or new)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Wrap bare URLs to satisfy markdownlint (MD034).

markdownlint flags bare URLs throughout the file; please wrap them in <...> or use link syntax (apply consistently to all entries).

🛠️ Example fix
- **URL:** https://github.com/BehaviorTree/BehaviorTree.CPP/issues/1029
+ **URL:** <https://github.com/BehaviorTree/BehaviorTree.CPP/issues/1029>
📝 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
### [x] #1029 - Mathematical order of operations in script is wrong
- **URL:** https://github.com/BehaviorTree/BehaviorTree.CPP/issues/1029
- **Summary:** `A - B + C` is evaluated as `A - (B + C)` instead of left-to-right. The expression parser applies incorrect associativity for addition and subtraction at the same precedence level.
- **Component:** `src/scripting/`
- **Test file:** `tests/gtest_scripting.cpp` (or new)
### [x] #923 - Out-of-bounds read in script validation
- **URL:** https://github.com/BehaviorTree/BehaviorTree.CPP/issues/923
- **Summary:** `ValidateScript` can create a `std::string` from a non-NULL-terminated fixed-size stack buffer when parsing invalid scripts that generate large error messages, causing `strlen` to read out of bounds.
- **Component:** `src/scripting/`
- **Test file:** `tests/gtest_scripting.cpp` (or new)
### [x] `#1029` - Mathematical order of operations in script is wrong
- **URL:** <https://github.com/BehaviorTree/BehaviorTree.CPP/issues/1029>
- **Summary:** `A - B + C` is evaluated as `A - (B + C)` instead of left-to-right. The expression parser applies incorrect associativity for addition and subtraction at the same precedence level.
- **Component:** `src/scripting/`
- **Test file:** `tests/gtest_scripting.cpp` (or new)
### [x] `#923` - Out-of-bounds read in script validation
- **URL:** <https://github.com/BehaviorTree/BehaviorTree.CPP/issues/923>
- **Summary:** `ValidateScript` can create a `std::string` from a non-NULL-terminated fixed-size stack buffer when parsing invalid scripts that generate large error messages, causing `strlen` to read out of bounds.
- **Component:** `src/scripting/`
- **Test file:** `tests/gtest_scripting.cpp` (or new)
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)

[warning] 13-13: Bare URL used

(MD034, no-bare-urls)


[warning] 19-19: Bare URL used

(MD034, no-bare-urls)

🤖 Prompt for AI Agents
In `@FIXME.md` around lines 12 - 23, Wrap all bare URLs in FIXME.md for the listed
entries (`#1029` and `#923`) to satisfy markdownlint MD034 by enclosing them in
angle brackets or using markdown link syntax; update both occurrences shown (the
URLs for the GitHub issues) so lines like - **URL:** https://github.com/...
become - **URL:** <https://github.com/...> (or [text](https://...)) and apply
the same wrapping consistently for any other bare URLs in the file.

Comment on lines +244 to +258
| Category | Count | Issue Numbers |
|----------|-------|---------------|
| Scripting / Script Parser | 3 | #1029, #923, #832 |
| Ports / Type Conversion | 5 | #1065, #982, #969, #948, #858 |
| Blackboard | 3 | #974, #942, #408 |
| Control Nodes (Parallel) | 3 | #1045, #1041, #966 |
| Control Nodes (Reactive) | 2 | #1031, #917 |
| XML Parsing | 4 | #979, #883, #880, #672 |
| BehaviorTreeFactory | 4 | #1046, #937, #934, #930 |
| Loggers | 1 | #1057 |
| Tree Execution | 1 | #686 |
| TreeNode | 1 | #861 |
| JSON Exporter | 1 | #989 |
| Build / Dependencies | 1 | #959 |
| Platform-Specific | 1 | #869 |
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix summary table pipe spacing (MD060).

The separator row should include spaces around pipes to satisfy the configured table style.

🛠️ Example fix
-|----------|-------|---------------|
+| -------- | ----- | ------------- |
📝 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
| Category | Count | Issue Numbers |
|----------|-------|---------------|
| Scripting / Script Parser | 3 | #1029, #923, #832 |
| Ports / Type Conversion | 5 | #1065, #982, #969, #948, #858 |
| Blackboard | 3 | #974, #942, #408 |
| Control Nodes (Parallel) | 3 | #1045, #1041, #966 |
| Control Nodes (Reactive) | 2 | #1031, #917 |
| XML Parsing | 4 | #979, #883, #880, #672 |
| BehaviorTreeFactory | 4 | #1046, #937, #934, #930 |
| Loggers | 1 | #1057 |
| Tree Execution | 1 | #686 |
| TreeNode | 1 | #861 |
| JSON Exporter | 1 | #989 |
| Build / Dependencies | 1 | #959 |
| Platform-Specific | 1 | #869 |
| Category | Count | Issue Numbers |
| -------- | ----- | ------------- |
| Scripting / Script Parser | 3 | `#1029`, `#923`, `#832` |
| Ports / Type Conversion | 5 | `#1065`, `#982`, `#969`, `#948`, `#858` |
| Blackboard | 3 | `#974`, `#942`, `#408` |
| Control Nodes (Parallel) | 3 | `#1045`, `#1041`, `#966` |
| Control Nodes (Reactive) | 2 | `#1031`, `#917` |
| XML Parsing | 4 | `#979`, `#883`, `#880`, `#672` |
| BehaviorTreeFactory | 4 | `#1046`, `#937`, `#934`, `#930` |
| Loggers | 1 | `#1057` |
| Tree Execution | 1 | `#686` |
| TreeNode | 1 | `#861` |
| JSON Exporter | 1 | `#989` |
| Build / Dependencies | 1 | `#959` |
| Platform-Specific | 1 | `#869` |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)

[warning] 245-245: Table column style
Table pipe is missing space to the right for style "compact"

(MD060, table-column-style)


[warning] 245-245: Table column style
Table pipe is missing space to the left for style "compact"

(MD060, table-column-style)


[warning] 245-245: Table column style
Table pipe is missing space to the right for style "compact"

(MD060, table-column-style)


[warning] 245-245: Table column style
Table pipe is missing space to the left for style "compact"

(MD060, table-column-style)


[warning] 245-245: Table column style
Table pipe is missing space to the right for style "compact"

(MD060, table-column-style)


[warning] 245-245: Table column style
Table pipe is missing space to the left for style "compact"

(MD060, table-column-style)

🤖 Prompt for AI Agents
In `@FIXME.md` around lines 244 - 258, The markdown table separator row lacks
spaces around the pipe characters; update the separator row beneath the header
"| Category | Count | Issue Numbers |" so each pipe has a space on both sides
(e.g., change "|----------|-------|---------------|" to a separator with spaces
around the pipes and hyphens for each column) to satisfy MD060 table style.

facontidavide and others added 5 commits January 31, 2026 09:11
…ations

parseString<T>() had two bugs for enum types:
1. Null pointer dereference on config().enums when getInput() was called
   before the factory assigned the scripting enum registry.
2. Enums with a convertFromString<T> specialization (e.g. NodeType,
   PortDirection) were never dispatched to it — the fallback tried
   convertFromString<int> which fails for named strings like "Control".

The fix adds a null check for config().enums, tries numeric parsing
via std::from_chars for integer enum values, then falls back to
convertFromString<T>() for types with user-provided specializations.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The bug (getInput throwing std::out_of_range when a port has a default
value but is not specified in XML) was already fixed in the current
code. getInputStamped now properly falls back to the manifest's default
value using find() instead of at(). Adding a regression test to prevent
future regressions.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…scripts

When blackboard values are stored as strings (e.g. via
blackboard->set(key, value.dump())), scripting comparisons between
two such entries used lexicographic ordering instead of numeric.
This caused "9" < "10" to evaluate as false.

The fix attempts numeric parsing via strtod when both operands in a
comparison are strings. If both parse as valid numbers, numeric
comparison is used; otherwise falls back to lexicographic comparison.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When a port declares a default blackboard remapping (e.g., "{=}") but
the XML doesn't explicitly specify the port, getLockedPortContent
returned an empty AnyPtrLocked because no blackboard entry existed.

The fix lazily creates the blackboard entry using the port's type info
from the manifest when the entry is missing but the remapped key is
valid. This makes getLockedPortContent consistent with the expectation
that default-remapped ports produce usable locked references.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
debugMessage() only printed the remapping direction for subtree entries
(e.g., "[local] remapped to port of parent tree [parent]") without
showing the type of the parent blackboard entry. Now it resolves the
parent entry and appends the type name, making the debug output useful
for diagnosing subtree port remapping issues.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@facontidavide facontidavide marked this pull request as draft January 31, 2026 08:32
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@include/behaviortree_cpp/tree_node.h`:
- Around line 436-454: The numeric parsing uses a fixed int (tmp) which can
truncate/wrap enums with different underlying types; change the temporary to the
enum's underlying type (use std::underlying_type_t<T>, possibly wrapped with
std::make_unsigned_t if you want unsigned parsing) and use that type with
std::from_chars, then static_cast<T>(tmp) on success; update the parsing block
that currently declares "int tmp" (and the std::from_chars call) to use the
underlying-type temporary so config().enums handling and the fallback to
convertFromString<T>(str) remain unchanged.

In `@src/tree_node.cpp`:
- Around line 482-496: The condition uses implicit pointer-to-bool conversion
for _p->config.manifest; change the check to an explicit null comparison so
clang-tidy passes. Update the if in the getAnyLocked/createEntry flow (the block
that checks result and manifest) to use (_p->config.manifest != nullptr) instead
of just _p->config.manifest; keep the rest of the logic around getAnyLocked,
manifest->ports.find(key), createEntry(bb_key, port_it->second) and the
subsequent getAnyLocked unchanged.

Comment on lines +436 to +454
// Check the ScriptingEnumsRegistry first, if available.
if(config().enums)
{
return static_cast<T>(it->second);
auto it = config().enums->find(str);
if(it != config().enums->end())
{
return static_cast<T>(it->second);
}
}
else
// Try numeric conversion (e.g. "2" for an enum value).
int tmp = 0;
auto [ptr, ec] = std::from_chars(str.data(), str.data() + str.size(), tmp);
if(ec == std::errc() && ptr == str.data() + str.size())
{
// hopefully str contains a number that can be parsed. May throw
return static_cast<T>(convertFromString<int>(str));
return static_cast<T>(tmp);
}
// Fall back to convertFromString<T>, which uses a user-provided
// specialization if one exists. Issue #948.
return convertFromString<T>(str);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, find and verify the file exists
git ls-files include/behaviortree_cpp/tree_node.h

Repository: BehaviorTree/BehaviorTree.CPP

Length of output: 109


🏁 Script executed:

# Check the exact code at lines 436-454
sed -n '430,460p' include/behaviortree_cpp/tree_node.h

Repository: BehaviorTree/BehaviorTree.CPP

Length of output: 1020


🏁 Script executed:

# Get more context around this function to understand the template signature
sed -n '420,460p' include/behaviortree_cpp/tree_node.h

Repository: BehaviorTree/BehaviorTree.CPP

Length of output: 1339


🏁 Script executed:

# Check C++ standard used in the project
grep -r "CMAKE_CXX_STANDARD\|std_version\|std11\|std14\|std17\|std20" CMakeLists.txt | head -20

Repository: BehaviorTree/BehaviorTree.CPP

Length of output: 99


🏁 Script executed:

# Verify std::underlying_type_t is available (C++11+)
rg "std::underlying_type" include/behaviortree_cpp/ --type cpp --type h

Repository: BehaviorTree/BehaviorTree.CPP

Length of output: 1859


Use the enum's underlying type for numeric parsing, not int.
Line 446 parses all enum values into int, which silently truncates or wraps values from enums with wider or unsigned underlying types.

Suggested fix
-    int tmp = 0;
-    auto [ptr, ec] = std::from_chars(str.data(), str.data() + str.size(), tmp);
+    using Underlying = std::underlying_type_t<T>;
+    Underlying tmp{};
+    auto [ptr, ec] = std::from_chars(str.data(), str.data() + str.size(), tmp);
🤖 Prompt for AI Agents
In `@include/behaviortree_cpp/tree_node.h` around lines 436 - 454, The numeric
parsing uses a fixed int (tmp) which can truncate/wrap enums with different
underlying types; change the temporary to the enum's underlying type (use
std::underlying_type_t<T>, possibly wrapped with std::make_unsigned_t if you
want unsigned parsing) and use that type with std::from_chars, then
static_cast<T>(tmp) on success; update the parsing block that currently declares
"int tmp" (and the std::from_chars call) to use the underlying-type temporary so
config().enums handling and the fallback to convertFromString<T>(str) remain
unchanged.

Comment on lines +482 to +496
const auto bb_key = std::string(*remapped_key);
auto result = _p->config.blackboard->getAnyLocked(bb_key);
if(!result && _p->config.manifest)
{
// Entry doesn't exist yet. Create it using the port's type info
// from the manifest so that getLockedPortContent works even when
// the port is not explicitly declared in XML. Issue #942.
auto port_it = _p->config.manifest->ports.find(key);
if(port_it != _p->config.manifest->ports.end())
{
_p->config.blackboard->createEntry(bb_key, port_it->second);
result = _p->config.blackboard->getAnyLocked(bb_key);
}
}
return result;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cat -n src/tree_node.cpp | sed -n '482,496p'

Repository: BehaviorTree/BehaviorTree.CPP

Length of output: 830


🌐 Web query:

clang-tidy readability-implicit-bool-conversion rule documentation

💡 Result:

readability-implicit-bool-conversion is a clang-tidy readability check that flags implicit conversions between built-in types and bool (e.g., intbool, pointer→bool, boolint/float) because they can hide bugs and/or reduce readability; it also provides fix-it hints (e.g., if (i)if (i != 0), if (p)if (p != nullptr), while (1)while (true)). [1]

Options (common)

  • AllowIntegerConditions (default 0): allow integer-to-bool in conditions. [1]
  • AllowPointerConditions (default 0): allow pointer-to-bool in conditions. [1]

Name note

This check was renamed from readability-implicit-bool-cast to readability-implicit-bool-conversion. [2]

How to set options

Via .clang-tidy or -config with CheckOptions, e.g. enabling pointer conditions: [3]

clang-tidy -config="{Checks: '-*,readability-implicit-bool-conversion',
  CheckOptions: [{key: readability-implicit-bool-conversion.AllowPointerConditions, value: 1}]}" \
  file.cpp -- -std=c++20

Sources: LLVM/Clang-Tidy docs for the check and its options [1], rename notice [2], and a working -config example [3].


Make the pointer check explicit to satisfy clang-tidy.
Line 484 uses implicit pointer-to-bool conversion on _p->config.manifest, triggering readability-implicit-bool-conversion.

Suggested fix
-    if(!result && _p->config.manifest)
+    if(!result && _p->config.manifest != nullptr)
🧰 Tools
🪛 GitHub Actions: pre-commit

[warning] 484-484: clang-tidy: readability-implicit-bool-conversion: implicit conversion from 'const TreeNodeManifest *' to 'bool' detected.

🤖 Prompt for AI Agents
In `@src/tree_node.cpp` around lines 482 - 496, The condition uses implicit
pointer-to-bool conversion for _p->config.manifest; change the check to an
explicit null comparison so clang-tidy passes. Update the if in the
getAnyLocked/createEntry flow (the block that checks result and manifest) to use
(_p->config.manifest != nullptr) instead of just _p->config.manifest; keep the
rest of the logic around getAnyLocked, manifest->ports.find(key),
createEntry(bb_key, port_it->second) and the subsequent getAnyLocked unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants