Repository navigation
Pick upstream script fixes for numeric strings (#1191, #1239, #1254, #1162) - #45
Conversation
Co-authored-by: Igor Stadnyk <fallenmi@users.noreply.github.com> (cherry picked from commit b4c070b)
…ehaviorTree#1239) The ~ operator in ExprUnaryArithmetic::evaluate guarded the cast of its double operand to int64_t with a hand-written range comparison. NaN passes it (every comparison is false) and so does exactly 2^63, because double(INT64_MAX) rounds up to 2^63; the following static_cast is then undefined behavior (UBSan float-cast-overflow). Fractional operands were also truncated silently, while the binary bitwise operators reject them. Use the checked Any::cast<int64_t> the binary bitwise operators already use, so these operands raise the existing RuntimeError instead. (cherry picked from commit ea8dafa)
…ript arithmetic (BehaviorTree#1246) (BehaviorTree#1254) Since BehaviorTree#1121 (shipped in 4.9.1), a numeric literal passed to a SubTree port was stored as int/int64_t/double. That locked the entry type, so the same value could no longer be read by a std::string port (tree creation failed), custom convertFromString<T> was bypassed, and the spelling was lost ("007" became 7). Literals are stored as strings again. The goal of BehaviorTree#1121, arithmetic on numeric literals in Script, is kept by the script evaluator instead: in + - * /, a string operand mixed with a number is converted when the whole string is a number (integer first, then double). Behavior change compared to 4.9.1 - 4.10.0, back to pre-4.9.1 behavior: two literal operands are strings ("1" + "2" is "12", "10" < "2" compares text), and compound assignment, unary minus, bitwise and logic operators on such a literal require an explicit number again. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 0388ee8) Fork adaptation: NumericStringToNumber uses std::from_chars with a full-consumption check instead of parseDouble, which upstream added in BehaviorTree#1167 (not in this fork). xml_parsing.cpp already stores SubTree literals as strings here, so that part of the change is a no-op. tests/gtest_subtree.cpp keeps this fork's SubTree tests (it accepts duplicate SubTree names, #41) and adds LiteralNumericPortsPreserveType and LiteralPortsAreReadByAnyPortType.
…haviorTree#1162) (cherry picked from commit b45100f)
…oNumber NumericStringToNumber came from upstream BehaviorTree#1254 with parseDouble, which this fork lacks (upstream BehaviorTree#1167), and was adapted to std::from_chars. Guard that call with __cpp_lib_to_chars like the fork's other double conversions, with a classic-locale stream as the fallback, so toolchains without floating-point from_chars still build.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Floating-point conversion bounds include/behaviortree_cpp/utils/convert_impl.hpp, include/behaviortree_cpp/utils/safe_any.hpp, tests/gtest_basic_types.cpp |
Floating-point conversions reject values at rounded integer upper bounds. Tests cover Any::cast and Blackboard::set at and within the tested boundaries. |
Script string conversion and operators include/behaviortree_cpp/scripting/operators.hpp, tests/script_parser_test.cpp, tests/gtest_subtree.cpp |
Arithmetic converts a string operand only when it is fully numeric and paired with a numeric operand. Logical NOT recognizes the specified boolean spellings, and unary complement uses checked int64_t conversion. Tests cover these operators and literal subtree port values. |
Possibly related PRs
- PickNikRobotics/BehaviorTree.CPP#44: Changes numeric-string parsing and script operators in the same implementation area.
Suggested reviewers: dvadovsz
Merge Risk: 
584e0
-
No concrete issue currently prevents merging; the floating-point parsing fallback remains untested on toolchains that use it.
Caution
Pre-merge checks failed
Please resolve all errors before merging. Addressing warnings is optional.
- Ignore (reviewers only)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Human Review Check |  |
The PR adds the public inline function BT::Ast::NumericStringToNumber(Any&) to include/behaviortree_cpp/scripting/operators.hpp. It also changes public-header conversion behavior in `convert_impl.… |
This PR requires review by a requested human reviewer. After review, a non-author requested reviewer should override this pre-merge check. |
-
Check name Status Explanation Description check The description follows the repository's expected structure in substance. It states the motivation, lists each cherry-picked upstream commit and the fork-specific conflict resolutions, and describes b… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description follows the repository's expected structure in substance. It states the motivation, lists each cherry-picked upstream commit and the fork-specific conflict resolutions, and describes behavior changes against 4.9.0-5noble, including a breaking change note for bitwise operators. It also reports the testing done (587/587 tests in RelWithDebInfo, ASan and UBSan, clean pre-commit) and notes the untested
#elsefallback branch. The template itself is a checklist of contributor guidance rather than sections with required content, so the description covers its intent: it explains the API/behavior change, the bug fixes, and the testing. The description does not explicitly confirm the template items about adding comments or running ./run_clang_tidy.sh, and it does not mention the CONTRIBUTORS_GUIDE.md review. Those are minor gaps.
-
Full details: Human Review Check
-
Explanation
The PR adds the public inline function
BT::Ast::NumericStringToNumber(Any&)toinclude/behaviortree_cpp/scripting/operators.hpp. It also changes public-header conversion behavior inconvert_impl.hppandsafe_any.hpp. This matches the explicit public API failure condition.Resolution
This PR requires review by a requested human reviewer. After review, a non-author requested reviewer should override this pre-merge check.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
CodeRabbit Verify
- Run CodeRabbit Verify to test the changes in this pull request.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
[written by AI]
Refs PickNikRobotics/moveit_pro#24179. Replaces #44, as agreed in #44 (comment).
Motivation
#44 changed Script operators in this fork only. Upstream BehaviorTree/BehaviorTree.CPP has since fixed the core of the same problem, numbers that reach scripts as strings through SubTree ports, so taking those commits narrows the fork's drift instead of widening it.
Brief description
Four upstream commits, cherry-picked with
-x, plus one fork commit:b4c070b4):!accepts string-backed booleans (!'true',!'0').ea8dafaa):~uses a checked conversion instead of an undefined cast.0388ee81): a whole-number string mixed with a number is used as that number in+ - * /(NumericStringToNumber, full-consumption parse). This replaces the fork's ownare_numbersfrom Enable interpretation of subtree ports as numbers. #23.b45100ff): float-to-integer casts at the numeric limit reject the rounded boundary. fix undefined double to int64 cast in the script complement operator BehaviorTree/BehaviorTree.CPP#1239's test of~9223372036854775808.0needs it.std::from_charsinNumericStringToNumberwith__cpp_lib_to_chars, with a classic-locale stream fallback.Fork-specific conflict resolutions, also in the commit messages:
NumericStringToNumberusesstd::from_charswith a full-consumption check where upstream callsparseDouble(str, dbl_val, true), which upstream added in Add macOS CI and fix Apple libc++ build BehaviorTree/BehaviorTree.CPP#1167 (macOS / Apple libc++). This fork doesn't have Add macOS CI and fix Apple libc++ build BehaviorTree/BehaviorTree.CPP#1167.src/xml_parsing.cpp: kept the fork's side. The fork already stores SubTree literals as strings, so that part of Store SubTree literals as strings again; numeric strings work in script arithmetic (#1246) BehaviorTree/BehaviorTree.CPP#1254 changes nothing here.tests/gtest_subtree.cpp: kept the fork's tests and added the tests these commits introduce. Upstream'sNestedDuplicateNames_ShouldFailis left out because this fork accepts duplicate SubTree names (fix(xml): accept SubTrees that share a name, as in 4.7.2 #41).Behavior changes vs.
4.9.0-5noblecompatibility-bot(moveit_pro reviewer role) built both and ran the same scripts against each. 0 of the 109 Script and precondition expressions shipped in moveit_pro and moveit_pro_example_ws behave differently.Breaking Change:note:&,|,^no longer convert a numeric string mixed with a number. A SubTree literalmask="6"inmask & 3gave2and now fails withOperation not permitted. This matches upstream and was accepted for parity; the fix for a customer script is(mask + 0) & 3.+ - * /no longer use only the leading digits of a string:'45degree_runs/' + 7gave52,'0x10' + 1gave1. These now fail...between a number and a numeric string concatenates (count .. '5') instead of returning no value.!works on string-backed booleans.~rejects fractional, NaN and out-of-range values instead of truncating them.Still not fixed upstream either, to be sent there as PRs and picked up with a later sync:
1 .. 2returning an empty value,'1' - '2'on two numeric strings, prefix parsing in comparisons,..printing2.000000.How it was tested
Built in a container from the
moveit-pro-devimage (clang with libstdc++ 13),BTCPP_GROOT_INTERFACE=OFF.main-picknikbefore, plus the upstream tests these commits bring) in RelWithDebInfo, ASan and UBSan builds.pre-commit(clang-format 17, clang-tidy, codespell) clean on the changed files.#elsebranch of the new guard isn't exercised: every toolchain here has floating-pointfrom_chars.The upstream commits are in upstream order except BehaviorTree#1162, which came last. BehaviorTree#1239's
~9223372036854775808.0test fails at that commit and passes once BehaviorTree#1162 is applied.