diff --git a/include/behaviortree_cpp/scripting/operators.hpp b/include/behaviortree_cpp/scripting/operators.hpp index dc747609e..483bc7f4d 100644 --- a/include/behaviortree_cpp/scripting/operators.hpp +++ b/include/behaviortree_cpp/scripting/operators.hpp @@ -15,12 +15,8 @@ #include "behaviortree_cpp/scripting/any_types.hpp" #include "behaviortree_cpp/scripting/script_parser.hpp" -#include -#include #include -#include #include -#include #include #include @@ -54,39 +50,6 @@ inline double StringToDouble(const Any& value, const Environment& env) return value.cast(); } -// Replace a string holding a whole number (e.g. "42" or "1.5") with that number. -// Unlike StringToDouble, it rejects numeric prefixes, booleans and enums. -inline void NumericStringToNumber(Any& value) -{ - const auto str = value.cast(); - const char* end = str.data() + str.size(); - int64_t int_val = 0; - const auto [ptr, ec] = std::from_chars(str.data(), end, int_val); - double dbl_val = 0; - if(ec == std::errc() && ptr == end) - { - value = Any(int_val); - return; - } - // Upstream calls parseDouble(str, dbl_val, true), which its #1167 added and this fork doesn't - // have yet: from_chars with a full-consumption check, or a classic-locale stream without it. -#if __cpp_lib_to_chars >= 201611L - const auto [dbl_ptr, dbl_ec] = std::from_chars(str.data(), end, dbl_val); - const bool parsed = dbl_ec == std::errc() && dbl_ptr == end; -#else - std::istringstream stream(str); - stream.imbue(std::locale::classic()); - stream >> dbl_val; - const bool parsed = !str.empty() && - !std::isspace(static_cast(str.front())) && - !stream.fail() && stream.peek() == std::char_traits::eof(); -#endif - if(parsed) - { - value = Any(dbl_val); - } -} - struct ExprBase { using Ptr = std::shared_ptr; @@ -166,38 +129,18 @@ struct ExprUnaryArithmetic : ExprBase case negate: return Any(-rv); case complement: - // Use the checked conversion, like the binary bitwise operators do. - // A plain static_cast is undefined for NaN and for values at or - // beyond the int64_t range: the hand-written comparison used here - // before let NaN (every comparison is false) and 2^63 (not greater - // than double(INT64_MAX), which rounds up to 2^63) through, and - // silently truncated fractional values. - try - { - return Any(static_cast(~rhs_v.cast())); - } - catch(const std::exception&) + if(rv > static_cast(std::numeric_limits::max()) || + rv < static_cast(std::numeric_limits::min())) { throw RuntimeError("Number out of range for bitwise operation"); } + return Any(static_cast(~static_cast(rv))); case logical_not: return Any(static_cast(!static_cast(rv))); } } else if(rhs_v.isString()) { - if(op == logical_not) - { - const auto str = rhs_v.cast(); - if(str == "true" || str == "True" || str == "TRUE" || str == "1") - { - return Any(0.0); - } - if(str == "false" || str == "False" || str == "FALSE" || str == "0") - { - return Any(1.0); - } - } throw RuntimeError("Invalid operator for std::string"); } throw RuntimeError("ExprUnaryArithmetic: undefined"); @@ -270,21 +213,11 @@ struct ExprBinaryArithmetic : ExprBase throw RuntimeError(ErrorNotInit("right", opStr())); } - // A numeric string mixed with a number is used as that number, e.g. the - // literal SubTree port int_val="42" in "int_val + 1" - if(op == plus || op == minus || op == times || op == div) - { - if(lhs_v.isString() && rhs_v.isNumber()) - { - NumericStringToNumber(lhs_v); - } - else if(lhs_v.isNumber() && rhs_v.isString()) - { - NumericStringToNumber(rhs_v); - } - } - - if(rhs_v.isNumber() && lhs_v.isNumber()) + // If just one value is strongly typed as a number, and the other can be casted to one, do arithmetic. If both are not strongly typed, continue. In the case of `+`, non-strongly typed values will result in concatenation. + bool are_numbers = (rhs_v.isNumber() && lhs_v.isNumber()) || + ((rhs_v.tryCast().has_value() && lhs_v.isNumber()) != + (rhs_v.isNumber() && lhs_v.tryCast().has_value())); + if(are_numbers) { auto lv = lhs_v.cast(); auto rv = rhs_v.cast(); diff --git a/include/behaviortree_cpp/utils/convert_impl.hpp b/include/behaviortree_cpp/utils/convert_impl.hpp index e32eefbcf..2a9f7c34c 100644 --- a/include/behaviortree_cpp/utils/convert_impl.hpp +++ b/include/behaviortree_cpp/utils/convert_impl.hpp @@ -114,15 +114,8 @@ inline void checkTruncation(const From& from) // Handle floating point to integer else if constexpr(std::is_floating_point_v && std::is_integral_v) { - // static_cast(To::max) rounds up to an out-of-range value when To has - // more significant bits than From's mantissa (e.g. double -> int64_t). In - // that case a plain `>` lets the rounded boundary through and the cast to To - // below overflows, so use a half-open upper bound to reject it first. - constexpr bool max_rounds_up = - std::numeric_limits::digits > std::numeric_limits::digits; - const From max_limit = static_cast(std::numeric_limits::max()); - const bool over = max_rounds_up ? (from >= max_limit) : (from > max_limit); - if(over || from < static_cast(std::numeric_limits::lowest()) || + if(from > static_cast(std::numeric_limits::max()) || + from < static_cast(std::numeric_limits::lowest()) || from != std::nearbyint(from)) { throw std::runtime_error("Invalid floating point to integer conversion"); diff --git a/include/behaviortree_cpp/utils/safe_any.hpp b/include/behaviortree_cpp/utils/safe_any.hpp index 65c6cc022..3a87caf8f 100644 --- a/include/behaviortree_cpp/utils/safe_any.hpp +++ b/include/behaviortree_cpp/utils/safe_any.hpp @@ -259,16 +259,8 @@ inline bool ValidCast(const SRC& val) // Handle conversion to integral else if constexpr(std::is_integral_v) { - // static_cast(TO::max) rounds up to an out-of-range value when TO has - // more significant bits than a floating SRC's mantissa (e.g. double -> - // int64_t). A plain `>` would let that rounded boundary through and the - // cast to TO below overflows, so use a half-open upper bound in that case. - constexpr bool max_rounds_up = - std::is_floating_point_v && - std::numeric_limits::digits > std::numeric_limits::digits; - const SRC max_limit = static_cast(std::numeric_limits::max()); - const bool over = max_rounds_up ? (val >= max_limit) : (val > max_limit); - if(over || val < static_cast(std::numeric_limits::lowest())) + if(val > static_cast(std::numeric_limits::max()) || + val < static_cast(std::numeric_limits::lowest())) { return false; } diff --git a/tests/gtest_basic_types.cpp b/tests/gtest_basic_types.cpp index 680c45ce3..57f8bafca 100644 --- a/tests/gtest_basic_types.cpp +++ b/tests/gtest_basic_types.cpp @@ -123,31 +123,6 @@ TEST(BasicTypes, ConvertFromString_Double) ASSERT_THROW((void)convertFromString("not_a_number"), RuntimeError); } -TEST(BasicTypes, DoubleToIntBoundary) -{ - // A double that lands exactly on 2^63 must be rejected before the cast, not - // static_cast to int64_t (which is out of range and undefined behavior). - // 2^63 = 9223372036854775808.0; INT64_MAX is 9223372036854775807. - const double two_pow_63 = 9223372036854775808.0; - EXPECT_ANY_THROW((void)BT::Any(two_pow_63).cast()); - - // 2^64 lands one past UINT64_MAX and must likewise be rejected. - const double two_pow_64 = 18446744073709551616.0; - EXPECT_ANY_THROW((void)BT::Any(two_pow_64).cast()); - - // In-range integral doubles still convert cleanly. - const double in_range = 4611686018427387904.0; // 2^62 - EXPECT_EQ(BT::Any(in_range).cast(), int64_t(4611686018427387904LL)); - EXPECT_EQ(BT::Any(1000.0).cast(), int64_t(1000)); - - // Same boundary reached through Blackboard::set, which routes the numeric - // safe-cast check (isCastingSafe/ValidCast) instead of Any::convert. - auto bb = BT::Blackboard::create(); - bb->set("k", int64_t(1)); // lock the entry to int64_t - EXPECT_ANY_THROW(bb->set("k", two_pow_63)); - EXPECT_NO_THROW(bb->set("k", in_range)); -} - TEST(BasicTypes, ConvertFromString_Bool) { ASSERT_TRUE(convertFromString("true")); diff --git a/tests/gtest_subtree.cpp b/tests/gtest_subtree.cpp index 185937fc3..f543de7bf 100644 --- a/tests/gtest_subtree.cpp +++ b/tests/gtest_subtree.cpp @@ -1089,175 +1089,3 @@ TEST(SubTree, NoNameAttribute_AutoGeneratesUniquePaths) auto status = tree.tickWhileRunning(); ASSERT_EQ(status, NodeStatus::SUCCESS); } - -// Regression test: logical NOT should accept literal boolean values passed to -// subtrees without changing their string representation in the blackboard. -TEST(SubTree, LiteralBooleanPortsSupportLogicalNot) -{ - // clang-format off - static const char* xml_text = R"( - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -)"; - // clang-format on - - BehaviorTreeFactory factory; - auto tree = factory.createTreeFromText(xml_text); - tree.rootBlackboard()->set("enabled_numeric", std::string("1")); - tree.rootBlackboard()->set("disabled_numeric", std::string("0")); - - ASSERT_EQ(tree.tickWhileRunning(), NodeStatus::SUCCESS); - ASSERT_EQ(tree.subtrees[1]->blackboard->get("enabled"), "true"); - ASSERT_EQ(tree.subtrees[1]->blackboard->get("disabled"), "false"); - ASSERT_EQ(tree.subtrees[1]->blackboard->get("enabled_upper"), "TRUE"); - ASSERT_EQ(tree.subtrees[1]->blackboard->get("disabled_title"), "False"); -} - -TEST(SubTree, LiteralNonBooleanPortsRejectLogicalNot) -{ - // clang-format off - static const char* xml_text = R"( - - - - - - - - - - - -)"; - // clang-format on - - BehaviorTreeFactory factory; - auto tree = factory.createTreeFromText(xml_text); - - ASSERT_THROW((void)tree.tickWhileRunning(), RuntimeError); - ASSERT_EQ(tree.subtrees[1]->blackboard->get("value"), "1not_bool"); -} - -// Regression test (#1121): Script expressions can do arithmetic with literal -// numeric values passed to subtrees. -TEST(SubTree, LiteralNumericPortsPreserveType) -{ - // clang-format off - static const char* xml_text = R"( - - - - - - - - - - - - - - - - - -)"; - // clang-format on - - BehaviorTreeFactory factory; - - auto tree = factory.createTreeFromText(xml_text); - - // Set the remapped parent value as an integer - tree.rootBlackboard()->set("from_parent", 100); - - const auto status = tree.tickWhileRunning(); - ASSERT_EQ(status, NodeStatus::SUCCESS); -} - -// Issue #1246: a literal passed to a SubTree stays a string, so ports of different -// types can read it, and its spelling is preserved. -TEST(SubTree, LiteralPortsAreReadByAnyPortType) -{ - // clang-format off - static const char* xml_text = R"( - - - - - - - - - - - - - - - - -)"; - // clang-format on - - BehaviorTreeFactory factory; - std::vector strings; - int int_val = 0; - double dbl_val = 0; - factory.registerSimpleAction("ReadString", - [&](TreeNode& node) { - strings.push_back( - node.getInput("value").value()); - return NodeStatus::SUCCESS; - }, - { InputPort("value") }); - factory.registerSimpleAction("ReadInt", - [&](TreeNode& node) { - int_val = node.getInput("value").value(); - return NodeStatus::SUCCESS; - }, - { InputPort("value") }); - factory.registerSimpleAction("ReadDouble", - [&](TreeNode& node) { - dbl_val = node.getInput("value").value(); - return NodeStatus::SUCCESS; - }, - { InputPort("value") }); - - auto tree = factory.createTreeFromText(xml_text); - ASSERT_EQ(tree.tickWhileRunning(), NodeStatus::SUCCESS); - ASSERT_EQ(strings, (std::vector{ "1", "007" })); - ASSERT_EQ(int_val, 1); - ASSERT_EQ(dbl_val, 1.0); -} diff --git a/tests/script_parser_test.cpp b/tests/script_parser_test.cpp index c2c0ccc5c..64ed0b1c2 100644 --- a/tests/script_parser_test.cpp +++ b/tests/script_parser_test.cpp @@ -323,37 +323,6 @@ TEST(ParserTest, Equations) EXPECT_EQ(GetResult(" par1:='3'; par2:=4; par1!=par2").cast(), 1); } -// Issue #1246: a numeric string mixed with a number is used as that number -TEST(ParserTest, NumericStringArithmetic) -{ - BT::Ast::Environment environment = { BT::Blackboard::create(), {} }; - - auto GetResult = [&environment](const char* text) -> BT::Any { - return GetScriptResult(environment, text); - }; - environment.vars->set("int_str", std::string("42")); - environment.vars->set("dbl_str", std::string("1.5")); - environment.vars->set("text", std::string("hello")); - - EXPECT_EQ(GetResult("int_str + 1").cast(), 43.0); - EXPECT_EQ(GetResult("1 + int_str").cast(), 43.0); - EXPECT_EQ(GetResult("int_str - 2").cast(), 40.0); - EXPECT_EQ(GetResult("int_str * 2").cast(), 84.0); - EXPECT_EQ(GetResult("3 / dbl_str").cast(), 2.0); - EXPECT_EQ(GetResult("'1e3' + 1").cast(), 1001.0); - - // the whole string must be a number - EXPECT_ANY_THROW(GetResult("text + 1")); - EXPECT_ANY_THROW(GetResult("'42abc' + 1")); - EXPECT_ANY_THROW(GetResult("'true' + 1")); - EXPECT_ANY_THROW(GetResult("' 42' + 1")); - - // unchanged: two strings concatenate, bitwise operators require numbers - EXPECT_EQ(GetResult("int_str + int_str").cast(), "4242"); - EXPECT_EQ(GetResult("int_str .. 1").cast(), "421"); - EXPECT_ANY_THROW(GetResult("int_str & 1")); -} - TEST(ParserTest, NotInitializedComparison) { BT::Ast::Environment environment = { BT::Blackboard::create(), {} }; @@ -754,26 +723,6 @@ TEST(ParserTest, UnaryOperators) EXPECT_EQ(Parse("10 + -3").value().cast(), 7.0); } -TEST(ParserTest, BitwiseComplementRejectsNonIntegers) -{ - BT::Ast::Environment env = { BT::Blackboard::create(), {} }; - auto Parse = [&env](const char* str) { return BT::ParseScriptAndExecute(env, str); }; - - // The operand was cast to int64_t after a hand-written range check that - // accepted NaN and exactly 2^63 (undefined conversion, reported by UBSan) - // and silently truncated fractional values. - EXPECT_FALSE(Parse("~(0.0 / 0.0)").has_value()); - EXPECT_FALSE(Parse("~9223372036854775808.0").has_value()); // 2^63 - EXPECT_FALSE(Parse("~1.5").has_value()); - EXPECT_FALSE(Parse("~(1.0 / 0.0)").has_value()); - - // Integral values, including the int64_t boundaries, still work. - EXPECT_EQ(Parse("~0").value().cast(), ~int64_t(0)); - EXPECT_EQ(Parse("~(2 * 3)").value().cast(), ~int64_t(6)); - EXPECT_EQ(Parse("~-9223372036854775808.0").value().cast(), - static_cast(~std::numeric_limits::min())); -} - TEST(ParserTest, TernaryExpressions) { BT::Ast::Environment env = { BT::Blackboard::create(), {} };