From f3c2b2d02c6737181aec3d4041eb3ddb16288dff Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Thu, 12 Nov 2026 20:10:14 +0100 Subject: [PATCH 1/6] Fix GCC 15 -Wnull-dereference in public headers, build with -Werror (#1288) Downstream packages (e.g. Nav2) compile BT.CPP headers with -Werror. GCC 15 at -O2 reports -Wnull-dereference in: - ThreadSafeSignal::notify(): false positive in vector copy-assignment; copy-construct the snapshot instead. - JsonExporter converters: *castPtr() dereferenced without a null check; throw LogicError on an empty / mismatched Any instead of UB. Also mark the static BT_RegisterNodesFromPlugin [[maybe_unused]] for non-plugin builds. To keep this from regressing, the library and every target that consumes its headers (sample_nodes, tests, tools, examples, fuzzers) now always build with -Wall -Wextra -Wpedantic -Wnull-dereference -Werror (MSVC: /W4 /WX /permissive-). Vendored 3rdparty code is not affected. Fix the existing warnings in tests, examples and fuzzers, including a stray "#pragma GCC diagnostic pop" in xml_parsing.cpp and unchecked get_if/dynamic_cast results in tests. CI: ros2-lyrical (GCC 15) now builds in Release, so -Wnull-dereference is active there. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ros2-lyrical.yaml | 7 +++- CMakeLists.txt | 20 +++++++---- examples/ex03_sqlite_log.cpp | 2 +- examples/t10_observer.cpp | 2 +- fuzzing/bb_fuzzer.cpp | 10 +++--- fuzzing/bt_fuzzer.cpp | 7 ++-- fuzzing/script_fuzzer.cpp | 2 +- include/behaviortree_cpp/bt_factory.h | 2 +- include/behaviortree_cpp/json_export.h | 22 ++++++++++-- .../utils/thread_safe_signal.h | 9 ++--- src/xml_parsing.cpp | 4 --- tests/gtest_any.cpp | 36 +++++++++---------- tests/gtest_blackboard.cpp | 5 ++- tests/gtest_decorator.cpp | 2 +- tests/gtest_duplicate_tree.cpp | 4 +-- tests/gtest_factory.cpp | 4 +-- tests/gtest_groot2_publisher_integration.cpp | 2 +- tests/gtest_parallel.cpp | 15 +++++--- tests/gtest_postconditions.cpp | 2 +- tests/gtest_preconditions.cpp | 1 + tests/gtest_simple_string.cpp | 6 ++-- tests/gtest_substitution.cpp | 3 ++ tests/gtest_subtree.cpp | 6 ++-- tests/gtest_switch.cpp | 30 ---------------- tests/gtest_try_catch.cpp | 17 +++++---- tests/include/animal_hierarchy_test.h | 15 ++++---- tests/script_parser_test.cpp | 2 +- 27 files changed, 124 insertions(+), 113 deletions(-) diff --git a/.github/workflows/ros2-lyrical.yaml b/.github/workflows/ros2-lyrical.yaml index 9f997af2b..c85d2f37e 100644 --- a/.github/workflows/ros2-lyrical.yaml +++ b/.github/workflows/ros2-lyrical.yaml @@ -20,9 +20,14 @@ jobs: # which configures with -DCMAKE_INSTALL_LIBDIR=lib/. # AFTER_SCRIPT asserts that the library is still installed to # $prefix/lib, as required by ament tooling (see issue #1175). + # Release: downstream packages (e.g. Nav2) build our headers with + # -Werror on this GCC; -Wnull-dereference only fires with + # optimization enabled (see issue #1288). - ROS_DISTRO: lyrical ROS_REPO: main - TARGET_CMAKE_ARGS: -DCMAKE_INSTALL_LIBDIR=lib/x86_64-linux-gnu + TARGET_CMAKE_ARGS: >- + -DCMAKE_INSTALL_LIBDIR=lib/x86_64-linux-gnu + -DCMAKE_BUILD_TYPE=Release AFTER_SCRIPT: >- lib=$(find "$BASEDIR/target_ws/install" -name libbehaviortree_cpp.so); echo "libbehaviortree_cpp.so installed at: ${lib:-NOT FOUND}"; diff --git a/CMakeLists.txt b/CMakeLists.txt index 442540383..c0c488185 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -288,16 +288,24 @@ target_compile_features(${BTCPP_LIBRARY} PUBLIC cxx_std_17) if(MSVC) target_compile_options(${BTCPP_LIBRARY} PRIVATE "/source-charset:utf-8") -else() - if(ENABLE_DEBUG) - target_compile_options(${BTCPP_LIBRARY} PRIVATE -Wall -Wextra -g3 -ggdb3 -O0 -fno-omit-frame-pointer) - else() - target_compile_options(${BTCPP_LIBRARY} PRIVATE -Wall -Wextra) - endif() +elseif(ENABLE_DEBUG) + target_compile_options(${BTCPP_LIBRARY} PRIVATE -g3 -ggdb3 -O0 -fno-omit-frame-pointer) endif() add_library(BT::${BTCPP_LIBRARY} ALIAS ${BTCPP_LIBRARY}) +# Downstream projects (e.g. Nav2) compile our public headers with -Werror, so +# the library and every target below that instantiates them must be +# warning-free (issue #1288). -Wnull-dereference only fires with optimization. +# Set after the 3rdparty subdirectories, so vendored code is not affected. +if(MSVC) + set(BTCPP_WARNING_FLAGS /W4 /WX /permissive-) +else() + set(BTCPP_WARNING_FLAGS -Wall -Wextra -Wpedantic -Wnull-dereference -Werror) +endif() +target_compile_options(${BTCPP_LIBRARY} PRIVATE ${BTCPP_WARNING_FLAGS}) +add_compile_options(${BTCPP_WARNING_FLAGS}) + # Add fuzzing targets if(ENABLE_FUZZING) add_fuzzing_targets() diff --git a/examples/ex03_sqlite_log.cpp b/examples/ex03_sqlite_log.cpp index 7535cf160..1c1d0434a 100644 --- a/examples/ex03_sqlite_log.cpp +++ b/examples/ex03_sqlite_log.cpp @@ -113,7 +113,7 @@ int main() // Write some data (from the blackboard) and write into the // extra column called "extra_data". We will use JSON serialization - auto meta_callback = [&](BT::Duration timestamp, const BT::TreeNode& node, + auto meta_callback = [&](BT::Duration /*timestamp*/, const BT::TreeNode& node, BT::NodeStatus prev_status, BT::NodeStatus status) -> std::string { if(prev_status == BT::NodeStatus::RUNNING && BT::isStatusCompleted(status)) diff --git a/examples/t10_observer.cpp b/examples/t10_observer.cpp index 622c39ded..449f9bb09 100644 --- a/examples/t10_observer.cpp +++ b/examples/t10_observer.cpp @@ -64,7 +64,7 @@ int main() tree.tickWhileRunning(); // You can access a specific statistic, using is full path or the UID - const auto& last_action_stats = observer.getStatistics("last_action"); + [[maybe_unused]] const auto& last_action_stats = observer.getStatistics("last_action"); assert(last_action_stats.transitions_count > 0); std::cout << "----------------" << std::endl; diff --git a/fuzzing/bb_fuzzer.cpp b/fuzzing/bb_fuzzer.cpp index 912693d22..779c1e227 100644 --- a/fuzzing/bb_fuzzer.cpp +++ b/fuzzing/bb_fuzzer.cpp @@ -94,7 +94,7 @@ class BlackboardFuzzer break; case 6: { // Try to get non-existent key - bb->get(generateKey()); + (void)bb->get(generateKey()); break; } } @@ -112,16 +112,16 @@ class BlackboardFuzzer bb->unset(existing_key); break; case 1: - bb->getEntry(existing_key); + (void)bb->getEntry(existing_key); break; case 2: - bb->get(existing_key); + (void)bb->get(existing_key); break; case 3: - bb->get(existing_key); + (void)bb->get(existing_key); break; case 4: - bb->get(existing_key); + (void)bb->get(existing_key); break; } } diff --git a/fuzzing/bt_fuzzer.cpp b/fuzzing/bt_fuzzer.cpp index 08ec0cb49..7b05c14d5 100644 --- a/fuzzing/bt_fuzzer.cpp +++ b/fuzzing/bt_fuzzer.cpp @@ -78,7 +78,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) std::string random_xml = fdp.ConsumeRandomLengthString(size - 1); try { - factory.createTreeFromText(random_xml); + (void)factory.createTreeFromText(random_xml); } catch(const std::exception&) {} @@ -105,7 +105,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) switch(fdp.ConsumeIntegralInRange(0, 2)) { case 0: - factory.createTreeFromText(xml, blackboard); + (void)factory.createTreeFromText(xml, blackboard); break; case 1: BT::VerifyXML(xml, {}); @@ -114,7 +114,8 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) factory.registerBehaviorTreeFromText(xml); if(!factory.registeredBehaviorTrees().empty()) { - factory.createTree(factory.registeredBehaviorTrees().front(), blackboard); + (void)factory.createTree(factory.registeredBehaviorTrees().front(), + blackboard); } break; } diff --git a/fuzzing/script_fuzzer.cpp b/fuzzing/script_fuzzer.cpp index 904b8ab55..a927f0f4a 100644 --- a/fuzzing/script_fuzzer.cpp +++ b/fuzzing/script_fuzzer.cpp @@ -50,7 +50,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) if(result.isNumber()) { - volatile auto num = result.cast(); + [[maybe_unused]] volatile auto num = result.cast(); } env.vars->set("result", result); diff --git a/include/behaviortree_cpp/bt_factory.h b/include/behaviortree_cpp/bt_factory.h index 9843f40ee..80c7924fd 100644 --- a/include/behaviortree_cpp/bt_factory.h +++ b/include/behaviortree_cpp/bt_factory.h @@ -65,7 +65,7 @@ inline TreeNodeManifest CreateManifest(const std::string& ID, #endif #else -#define BTCPP_EXPORT static +#define BTCPP_EXPORT [[maybe_unused]] static #endif /* Use this macro to automatically register one or more custom Nodes * into a factory. For instance: diff --git a/include/behaviortree_cpp/json_export.h b/include/behaviortree_cpp/json_export.h index aa5183a88..b8a7e38e3 100644 --- a/include/behaviortree_cpp/json_export.h +++ b/include/behaviortree_cpp/json_export.h @@ -17,6 +17,22 @@ namespace BT { +namespace details +{ +// Reference to the value of type T stored in entry; throws instead of +// dereferencing nullptr when entry is empty or holds another type. +template +const T& anyRef(const BT::Any& entry) +{ + const T* ptr = const_cast(entry).castPtr(); + if(ptr == nullptr) + { + throw LogicError("JsonExporter: BT::Any does not contain a ", demangle(typeid(T))); + } + return *ptr; +} +} // namespace details + /** * To add new type to the JSON library, you should follow these instructions: * https://json.nlohmann.me/features/arbitrary_types/ @@ -181,7 +197,7 @@ inline void JsonExporter::addConverter() type_names_.insert({ BT::demangle(typeid(T)), BT::TypeInfo::Create() }); ToJonConverter to_converter = [](const BT::Any& entry, nlohmann::json& dst) { - dst = *const_cast(entry).castPtr(); + dst = details::anyRef(entry); }; to_json_converters_.insert({ typeid(T), to_converter }); @@ -194,7 +210,7 @@ inline void JsonExporter::addConverter() //---- include vectors of T ToJonConverter to_array_converter = [](const BT::Any& entry, nlohmann::json& dst) { - dst = *const_cast(entry).castPtr>(); + dst = details::anyRef>(entry); }; to_json_converters_.insert({ typeid(std::vector), to_array_converter }); @@ -225,7 +241,7 @@ JsonExporter::addConverter(std::function func, //--------------------------------------------- // add the vector converter (must be created before moving converter) auto vector_converter = [converter](const BT::Any& entry, nlohmann::json& json) { - auto& vec = *const_cast(entry).castPtr>(); + const auto& vec = details::anyRef>(entry); for(const auto& item : vec) { nlohmann::json item_json; diff --git a/include/behaviortree_cpp/utils/thread_safe_signal.h b/include/behaviortree_cpp/utils/thread_safe_signal.h index 394a9ad30..482a09bb3 100644 --- a/include/behaviortree_cpp/utils/thread_safe_signal.h +++ b/include/behaviortree_cpp/utils/thread_safe_signal.h @@ -45,12 +45,13 @@ class ThreadSafeSignal void notify(CallableArgs... args) { - std::vector> subscribers; - { + // Copy-construct the snapshot: copy-assigning the vector triggers a + // -Wnull-dereference false positive in GCC 15 at -O2 (issue #1288). + const auto subscribers = [this] { const std::scoped_lock lock(mutex_); removeExpired(); - subscribers = subscribers_; - } + return subscribers_; + }(); for(const auto& weak_subscriber : subscribers) { if(auto subscriber = weak_subscriber.lock()) diff --git a/src/xml_parsing.cpp b/src/xml_parsing.cpp index 165dacab6..751b27475 100644 --- a/src/xml_parsing.cpp +++ b/src/xml_parsing.cpp @@ -297,10 +297,6 @@ struct XMLParser::PImpl void loadSubtreeModel(const XMLElement* xml_root); }; -#if defined(__linux) || defined(__linux__) -#pragma GCC diagnostic pop -#endif - XMLParser::XMLParser(const BehaviorTreeFactory& factory) : _p(new PImpl(factory)) {} diff --git a/tests/gtest_any.cpp b/tests/gtest_any.cpp index f09a21280..1b44c4947 100644 --- a/tests/gtest_any.cpp +++ b/tests/gtest_any.cpp @@ -110,13 +110,13 @@ TEST(Any, Cast) // Signed int. { Any a(42); - EXPECT_ANY_THROW(auto res = a.cast()); + EXPECT_ANY_THROW((void)a.cast()); EXPECT_EQ(a.cast(), 42); EXPECT_EQ(a.cast(), 42.0); EXPECT_EQ(a.cast(), "42"); Any b(-43); - EXPECT_ANY_THROW(auto res = b.cast()); + EXPECT_ANY_THROW((void)b.cast()); EXPECT_EQ(b.cast(), -43); EXPECT_EQ(b.cast(), -43.0); EXPECT_EQ(b.cast(), "-43"); @@ -139,7 +139,7 @@ TEST(Any, Cast) // unsigned int. { Any a(43u); - EXPECT_ANY_THROW(auto res = a.cast()); + EXPECT_ANY_THROW((void)a.cast()); EXPECT_EQ(a.cast(), 43u); EXPECT_EQ(a.cast(), 43); EXPECT_EQ(a.cast(), 43.0); @@ -182,7 +182,7 @@ TEST(Any, Cast) EXPECT_TRUE(b.cast()); EXPECT_EQ(typeid(b.cast()), typeid(bool)); // EXPECT_EQ(b.cast(), 44);? - EXPECT_ANY_THROW(auto res = b.cast()); + EXPECT_ANY_THROW((void)b.cast()); EXPECT_EQ(b.cast(), 44.1); #if __cpp_lib_to_chars >= 201611L const std::string b_str = b.cast(); @@ -196,13 +196,13 @@ TEST(Any, Cast) Any c(44.9); EXPECT_TRUE(c.cast()); // EXPECT_EQ(c.cast(), 44);? - EXPECT_ANY_THROW(auto res = c.cast()); + EXPECT_ANY_THROW((void)c.cast()); EXPECT_EQ(c.cast(), 44.9); Any d(-45.0); EXPECT_TRUE(c.cast()); // EXPECT_EQ(c.cast(), -45);? - EXPECT_ANY_THROW(auto res = c.cast()); + EXPECT_ANY_THROW((void)c.cast()); Any e(0.0); EXPECT_FALSE(e.cast()); @@ -212,18 +212,18 @@ TEST(Any, Cast) // String. { Any a(std::string{ "fifty" }); - EXPECT_ANY_THROW(auto res = a.cast()); - EXPECT_ANY_THROW(auto res = a.cast()); - EXPECT_ANY_THROW(auto res = a.cast()); + EXPECT_ANY_THROW((void)a.cast()); + EXPECT_ANY_THROW((void)a.cast()); + EXPECT_ANY_THROW((void)a.cast()); EXPECT_EQ(a.cast(), "fifty"); Any b(std::string{ "true" }); // EXPECT_TRUE(b.cast());? - EXPECT_ANY_THROW(auto res = b.cast()); + EXPECT_ANY_THROW((void)b.cast()); Any c(std::string{ "false" }); // EXPECT_FALSE(c.cast());? - EXPECT_ANY_THROW(auto res = c.cast()); + EXPECT_ANY_THROW((void)c.cast()); Any d(std::string{ "0" }); // EXPECT_FALSE(d.cast());? @@ -236,12 +236,12 @@ TEST(Any, Cast) EXPECT_EQ(e.cast(), 1.0); Any f(std::string{ "51" }); - EXPECT_ANY_THROW(auto res = f.cast()); + EXPECT_ANY_THROW((void)f.cast()); EXPECT_EQ(f.cast(), 51); EXPECT_EQ(f.cast(), 51.0); Any g(std::string{ "51.1" }); - EXPECT_ANY_THROW(auto res = g.cast()); + EXPECT_ANY_THROW((void)g.cast()); EXPECT_EQ(g.cast(), 51); EXPECT_DOUBLE_EQ(g.cast(), 51.1); } @@ -267,18 +267,18 @@ TEST(Any, StringToNumberIgnoresLocale) EXPECT_DOUBLE_EQ(Any(std::string("-0.25")).cast(), -0.25); EXPECT_FLOAT_EQ(Any(std::string("1.5e2")).cast(), 150.0f); EXPECT_EQ(Any(std::string("42")).cast(), 42); - EXPECT_ANY_THROW(auto res = Any(std::string("fifty")).cast()); + EXPECT_ANY_THROW((void)Any(std::string("fifty")).cast()); } TEST(Any, StringToUint16IsRangeChecked) { // A string that doesn't fit in uint16_t must be rejected, not wrapped. EXPECT_EQ(Any(std::string("65535")).cast(), 65535); - EXPECT_ANY_THROW(auto res = Any(std::string("65536")).cast()); - EXPECT_ANY_THROW(auto res = Any(std::string("70000")).cast()); - EXPECT_ANY_THROW(auto res = Any(std::string("-1")).cast()); + EXPECT_ANY_THROW((void)Any(std::string("65536")).cast()); + EXPECT_ANY_THROW((void)Any(std::string("70000")).cast()); + EXPECT_ANY_THROW((void)Any(std::string("-1")).cast()); auto bb = Blackboard::create(); bb->set("value", std::string("70000")); - EXPECT_ANY_THROW(auto res = bb->get("value")); + EXPECT_ANY_THROW((void)bb->get("value")); } diff --git a/tests/gtest_blackboard.cpp b/tests/gtest_blackboard.cpp index 9c72608d9..e6893a332 100644 --- a/tests/gtest_blackboard.cpp +++ b/tests/gtest_blackboard.cpp @@ -337,7 +337,10 @@ TEST(BlackboardTest, AnyPtrLockedSurvivesClear) auto locked = blackboard->getAnyLocked("value"); ASSERT_TRUE(bool(locked)); - blackboard->clear(); +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" + blackboard->clear(); // deprecated, but still under test +#pragma GCC diagnostic pop ASSERT_TRUE(blackboard->getKeyNames().empty()); ASSERT_EQ(locked.get()->cast(), 42); } diff --git a/tests/gtest_decorator.cpp b/tests/gtest_decorator.cpp index d6706dc7b..ed5c7ef66 100644 --- a/tests/gtest_decorator.cpp +++ b/tests/gtest_decorator.cpp @@ -100,7 +100,7 @@ struct TimeoutAndRetry : testing::Test BT::TimeoutNode timeout_root; BT::SyncActionTest action; - TimeoutAndRetry() : timeout_root("deadline", 9), retry("retry", 1000), action("action") + TimeoutAndRetry() : retry("retry", 1000), timeout_root("deadline", 9), action("action") { timeout_root.setChild(&retry); retry.setChild(&action); diff --git a/tests/gtest_duplicate_tree.cpp b/tests/gtest_duplicate_tree.cpp index 591d6a3dc..ef3798adc 100644 --- a/tests/gtest_duplicate_tree.cpp +++ b/tests/gtest_duplicate_tree.cpp @@ -155,8 +155,8 @@ TEST(DuplicateBehaviorTreeId, RepeatedInlineTextIsSilent) BehaviorTreeFactory factory; const std::string xml = minimalTreeXml("MainTree"); const auto output = captureStdout([&] { - factory.createTreeFromText(xml); - factory.createTreeFromText(xml); + (void)factory.createTreeFromText(xml); + (void)factory.createTreeFromText(xml); }); EXPECT_FALSE(hasWarning(output)) << output; } diff --git a/tests/gtest_factory.cpp b/tests/gtest_factory.cpp index 5ac4adccd..7d4f493a6 100644 --- a/tests/gtest_factory.cpp +++ b/tests/gtest_factory.cpp @@ -366,14 +366,14 @@ TEST(BehaviorTreeFactory, CreateTreeFromFileWhichIncludesItself) { BehaviorTreeFactory factory; std::string path = FilePath("trees/self_include.xml"); - ASSERT_THROW(factory.createTreeFromFile(path), RuntimeError); + ASSERT_THROW((void)factory.createTreeFromFile(path), RuntimeError); } TEST(BehaviorTreeFactory, CreateTreeFromFileWithCyclicIncludes) { BehaviorTreeFactory factory; std::string path = FilePath("trees/parent_include_child_cycle.xml"); - ASSERT_THROW(factory.createTreeFromFile(path), RuntimeError); + ASSERT_THROW((void)factory.createTreeFromFile(path), RuntimeError); // a failed load must not leave stale state behind in the factory's parser Tree tree = factory.createTreeFromFile(FilePath("trees/parent_include_child.xml")); diff --git a/tests/gtest_groot2_publisher_integration.cpp b/tests/gtest_groot2_publisher_integration.cpp index a6b9525fa..7d246fe06 100644 --- a/tests/gtest_groot2_publisher_integration.cpp +++ b/tests/gtest_groot2_publisher_integration.cpp @@ -207,7 +207,7 @@ TEST(Groot2PublisherIntegration, HookRequestRejectsNonIntegerUid) // get() used to wrap uid + 65536 and truncate uid + 0.5 to the uid // of the node, and to cast 1e300 out of range (undefined behavior). - for(const nlohmann::json bad_uid : + for(const nlohmann::json& bad_uid : { nlohmann::json(uid + 65536), nlohmann::json(uid + 0.5), nlohmann::json(1e300) }) { auto hook = Groot2Test::makeHook(uid, BT::Monitor::Hook::Mode::REPLACE, diff --git a/tests/gtest_parallel.cpp b/tests/gtest_parallel.cpp index 81279921a..2f8c99b45 100644 --- a/tests/gtest_parallel.cpp +++ b/tests/gtest_parallel.cpp @@ -426,17 +426,20 @@ TEST(Parallel, FailingParallel) BT::TestNodeConfig good_config; good_config.async_delay = std::chrono::milliseconds(200); good_config.return_status = NodeStatus::SUCCESS; - factory.registerNodeType("GoodTest", good_config); + factory.registerNodeType( + "GoodTest", std::make_shared(good_config)); BT::TestNodeConfig bad_config; bad_config.async_delay = std::chrono::milliseconds(100); bad_config.return_status = NodeStatus::FAILURE; - factory.registerNodeType("BadTest", bad_config); + factory.registerNodeType( + "BadTest", std::make_shared(bad_config)); BT::TestNodeConfig slow_config; slow_config.async_delay = std::chrono::milliseconds(300); slow_config.return_status = NodeStatus::SUCCESS; - factory.registerNodeType("SlowTest", slow_config); + factory.registerNodeType( + "SlowTest", std::make_shared(slow_config)); auto tree = factory.createTreeFromText(xml_text); BT::TreeObserver observer(tree); @@ -458,12 +461,14 @@ TEST(Parallel, ParallelAll) BT::TestNodeConfig good_config; good_config.async_delay = std::chrono::milliseconds(300); good_config.return_status = NodeStatus::SUCCESS; - factory.registerNodeType("GoodTest", good_config); + factory.registerNodeType( + "GoodTest", std::make_shared(good_config)); BT::TestNodeConfig bad_config; bad_config.async_delay = std::chrono::milliseconds(100); bad_config.return_status = NodeStatus::FAILURE; - factory.registerNodeType("BadTest", bad_config); + factory.registerNodeType( + "BadTest", std::make_shared(bad_config)); { const char* xml_text = R"( diff --git a/tests/gtest_postconditions.cpp b/tests/gtest_postconditions.cpp index 1135d5947..4861ee238 100644 --- a/tests/gtest_postconditions.cpp +++ b/tests/gtest_postconditions.cpp @@ -83,7 +83,7 @@ TEST(PostConditions, Issue601) BehaviorTreeFactory factory; auto tree = factory.createTreeFromText(xml_text); - const auto status = tree.tickWhileRunning(); + tree.tickWhileRunning(); ASSERT_EQ(tree.rootBlackboard()->get("test"), "halted"); } diff --git a/tests/gtest_preconditions.cpp b/tests/gtest_preconditions.cpp index 6a7473249..c3933b10a 100644 --- a/tests/gtest_preconditions.cpp +++ b/tests/gtest_preconditions.cpp @@ -376,6 +376,7 @@ TEST(Preconditions, Issue585) tree.tickWhileRunning(); auto coro = dynamic_cast(tree.subtrees.front()->nodes.back().get()); + ASSERT_NE(coro, nullptr); ASSERT_EQ(coro->times_ticked, 0); } diff --git a/tests/gtest_simple_string.cpp b/tests/gtest_simple_string.cpp index fc103e529..aed3688a9 100644 --- a/tests/gtest_simple_string.cpp +++ b/tests/gtest_simple_string.cpp @@ -152,7 +152,8 @@ TEST(SimpleStringTest, CopyAssignmentToDefault) TEST(SimpleStringTest, SelfCopyAssignment) { SimpleString s("test"); - s = s; + const auto& alias = s; // hide the self-assignment from -Wself-assign + s = alias; EXPECT_EQ(s.size(), 4); EXPECT_STREQ(s.data(), "test"); } @@ -215,7 +216,8 @@ TEST(SimpleStringTest, MoveAssignmentToDefault) TEST(SimpleStringTest, SelfMoveAssignment) { SimpleString s("test"); - s = std::move(s); + auto& alias = s; // hide the self-move from -Wself-move + s = std::move(alias); EXPECT_EQ(s.size(), 4); EXPECT_STREQ(s.data(), "test"); } diff --git a/tests/gtest_substitution.cpp b/tests/gtest_substitution.cpp index a1bcf9dd7..a8d20c2e6 100644 --- a/tests/gtest_substitution.cpp +++ b/tests/gtest_substitution.cpp @@ -60,16 +60,19 @@ TEST(Substitution, Parser) ASSERT_EQ(rules.count("actionD"), 1); auto configA = std::get_if(&rules.at("actionA")); + ASSERT_NE(configA, nullptr); ASSERT_EQ(configA->return_status, NodeStatus::SUCCESS); ASSERT_EQ(configA->async_delay, std::chrono::milliseconds(2000)); ASSERT_EQ(configA->post_script, "msg ='message SUBSTITUED'"); auto configB = std::get_if(&rules.at("actionB")); + ASSERT_NE(configB, nullptr); ASSERT_EQ(configB->return_status, NodeStatus::FAILURE); ASSERT_EQ(configB->async_delay, std::chrono::milliseconds(0)); ASSERT_TRUE(configB->post_script.empty()); auto configScript = std::get_if(&rules.at("actionD")); + ASSERT_NE(configScript, nullptr); ASSERT_FALSE(configScript->return_status_script.empty()); ASSERT_EQ(configScript->return_status_script, "(mock_should_fail == true) ? FAILURE : " "SUCCESS"); diff --git a/tests/gtest_subtree.cpp b/tests/gtest_subtree.cpp index 56cf1fbeb..30f6da08a 100644 --- a/tests/gtest_subtree.cpp +++ b/tests/gtest_subtree.cpp @@ -1344,7 +1344,7 @@ TEST(SubTree, DeclaredPortNameThatCannotBeBound) )"; BehaviorTreeFactory factory; - EXPECT_THROW(factory.createTreeFromText(xml_text), RuntimeError); + EXPECT_THROW((void)factory.createTreeFromText(xml_text), RuntimeError); } // The same underscore attribute on a SubTree that does not declare it is not a @@ -1367,7 +1367,7 @@ TEST(SubTree, UndeclaredUnderscoreAttributeIsNotAPort) )"; BehaviorTreeFactory factory; - EXPECT_NO_THROW(factory.createTreeFromText(xml_text)); + EXPECT_NO_THROW((void)factory.createTreeFromText(xml_text)); } // A declared port nobody remaps stays inert and keeps loading, so existing trees @@ -1390,7 +1390,7 @@ TEST(SubTree, DeclaredUnbindablePortLoadsWhenNotRemapped) )"; BehaviorTreeFactory factory; - EXPECT_NO_THROW(factory.createTreeFromText(xml_text)); + EXPECT_NO_THROW((void)factory.createTreeFromText(xml_text)); } namespace diff --git a/tests/gtest_switch.cpp b/tests/gtest_switch.cpp index 04f18c263..d208261ee 100644 --- a/tests/gtest_switch.cpp +++ b/tests/gtest_switch.cpp @@ -12,36 +12,6 @@ using BT::NodeStatus; using std::chrono::milliseconds; -namespace -{ - -const char* xml_text = R"( - - - - - - - - - - - - - )"; - -BT::NodeStatus TickWhileRunning(BT::TreeNode& node) -{ - auto status = node.executeTick(); - while(status == BT::NodeStatus::RUNNING) - { - status = node.executeTick(); - } - return status; -} - -} // namespace - struct SwitchTest : testing::Test { using Switch2 = BT::SwitchNode<2>; diff --git a/tests/gtest_try_catch.cpp b/tests/gtest_try_catch.cpp index ff245e44e..62296bc60 100644 --- a/tests/gtest_try_catch.cpp +++ b/tests/gtest_try_catch.cpp @@ -378,15 +378,14 @@ TEST_F(TryCatchTest, AsyncCatchCompletesInsideSequence) // ticking the catch child until it completes. const int kRunningTicks = 5; int catch_ticks = 0; - factory.registerSimpleCondition("AsyncCleanup", - [&catch_ticks, kRunningTicks](BT::TreeNode&) { - catch_ticks++; - if(catch_ticks <= kRunningTicks) - { - return NodeStatus::RUNNING; - } - return NodeStatus::SUCCESS; - }); + factory.registerSimpleCondition("AsyncCleanup", [&catch_ticks](BT::TreeNode&) { + catch_ticks++; + if(catch_ticks <= kRunningTicks) + { + return NodeStatus::RUNNING; + } + return NodeStatus::SUCCESS; + }); const std::string xml_text = R"( diff --git a/tests/include/animal_hierarchy_test.h b/tests/include/animal_hierarchy_test.h index 0454c6bf2..4b8a30d84 100644 --- a/tests/include/animal_hierarchy_test.h +++ b/tests/include/animal_hierarchy_test.h @@ -3,13 +3,14 @@ #include #include -// Animal hierarchy for testing polymorphic port connections -// -// Animal -// / \ -// Cat Dog -// | -// Sphynx +/* Animal hierarchy for testing polymorphic port connections + + Animal + / \ + Cat Dog + | + Sphynx +*/ class Animal { diff --git a/tests/script_parser_test.cpp b/tests/script_parser_test.cpp index 6715431b2..856e35f30 100644 --- a/tests/script_parser_test.cpp +++ b/tests/script_parser_test.cpp @@ -233,7 +233,7 @@ TEST(ParserTest, Equations) EXPECT_EQ(GetResult("y | x").cast(), (5 | 3)); EXPECT_EQ(GetResult("y ^ x").cast(), (5 ^ 3)); - EXPECT_ANY_THROW(auto res = GetResult("y ^ 5.1").cast()); + EXPECT_ANY_THROW((void)GetResult("y ^ 5.1").cast()); // test string variables EXPECT_EQ(GetResult("A:='hello'; B:=' '; C:='world'; A+B+C").cast(), "hell" From 8fea1726809cec32291c861883b6d470cac40127 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Thu, 12 Nov 2026 20:18:40 +0100 Subject: [PATCH 2/6] Fix warnings from MSVC /W4, GCC 13 and clangd; prefer std::ignore - MSVC: unreachable tail in ValidCast (int->float branch now falls through to the common round-trip check), int64_t narrowing in ConvertWithBoundCheck, potentially uninitialized timestamp in StatusChangeLogger, uint8_t status in FileLogger2, int64_t elapsed time in SqliteLogger, unused catch variable. - GCC 13: read the minitrace output with rdbuf() instead of istreambuf_iterator, which trips -Wnull-dereference in libstdc++ 13. - clangd: PLUGIN_SYMBOL is now an inline constexpr variable. - Use std::ignore instead of (void) to discard [[nodiscard]] results. Co-Authored-By: Claude Opus 5.5 --- fuzzing/bb_fuzzer.cpp | 11 +++--- fuzzing/bt_fuzzer.cpp | 9 ++--- include/behaviortree_cpp/bt_factory.h | 2 +- include/behaviortree_cpp/utils/safe_any.hpp | 3 +- src/basic_types.cpp | 2 +- src/loggers/abstract_logger.cpp | 2 +- src/loggers/bt_file_logger_v2.cpp | 2 +- src/loggers/bt_sqlite_logger.cpp | 2 +- src/xml_parsing.cpp | 2 +- tests/gtest_any.cpp | 37 +++++++++++---------- tests/gtest_duplicate_tree.cpp | 5 +-- tests/gtest_factory.cpp | 5 +-- tests/gtest_loggers.cpp | 6 ++-- tests/gtest_subtree.cpp | 7 ++-- tests/script_parser_test.cpp | 4 ++- 15 files changed, 54 insertions(+), 45 deletions(-) diff --git a/fuzzing/bb_fuzzer.cpp b/fuzzing/bb_fuzzer.cpp index 779c1e227..2002ee5ac 100644 --- a/fuzzing/bb_fuzzer.cpp +++ b/fuzzing/bb_fuzzer.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include @@ -94,7 +95,7 @@ class BlackboardFuzzer break; case 6: { // Try to get non-existent key - (void)bb->get(generateKey()); + std::ignore = bb->get(generateKey()); break; } } @@ -112,16 +113,16 @@ class BlackboardFuzzer bb->unset(existing_key); break; case 1: - (void)bb->getEntry(existing_key); + std::ignore = bb->getEntry(existing_key); break; case 2: - (void)bb->get(existing_key); + std::ignore = bb->get(existing_key); break; case 3: - (void)bb->get(existing_key); + std::ignore = bb->get(existing_key); break; case 4: - (void)bb->get(existing_key); + std::ignore = bb->get(existing_key); break; } } diff --git a/fuzzing/bt_fuzzer.cpp b/fuzzing/bt_fuzzer.cpp index 7b05c14d5..3ab22a03e 100644 --- a/fuzzing/bt_fuzzer.cpp +++ b/fuzzing/bt_fuzzer.cpp @@ -2,6 +2,7 @@ #include "behaviortree_cpp/xml_parsing.h" #include +#include #include @@ -78,7 +79,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) std::string random_xml = fdp.ConsumeRandomLengthString(size - 1); try { - (void)factory.createTreeFromText(random_xml); + std::ignore = factory.createTreeFromText(random_xml); } catch(const std::exception&) {} @@ -105,7 +106,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) switch(fdp.ConsumeIntegralInRange(0, 2)) { case 0: - (void)factory.createTreeFromText(xml, blackboard); + std::ignore = factory.createTreeFromText(xml, blackboard); break; case 1: BT::VerifyXML(xml, {}); @@ -114,8 +115,8 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) factory.registerBehaviorTreeFromText(xml); if(!factory.registeredBehaviorTrees().empty()) { - (void)factory.createTree(factory.registeredBehaviorTrees().front(), - blackboard); + std::ignore = + factory.createTree(factory.registeredBehaviorTrees().front(), blackboard); } break; } diff --git a/include/behaviortree_cpp/bt_factory.h b/include/behaviortree_cpp/bt_factory.h index 80c7924fd..e5640df25 100644 --- a/include/behaviortree_cpp/bt_factory.h +++ b/include/behaviortree_cpp/bt_factory.h @@ -88,7 +88,7 @@ inline TreeNodeManifest CreateManifest(const std::string& ID, BTCPP_EXPORT void BT_RegisterNodesFromPlugin(BT::BehaviorTreeFactory& factory) // NOLINTEND(cppcoreguidelines-macro-usage,bugprone-macro-parentheses) -constexpr const char* PLUGIN_SYMBOL = "BT_RegisterNodesFromPlugin"; +inline constexpr const char* PLUGIN_SYMBOL = "BT_RegisterNodesFromPlugin"; bool WildcardMatch(const std::string& str, StringView filter); diff --git a/include/behaviortree_cpp/utils/safe_any.hpp b/include/behaviortree_cpp/utils/safe_any.hpp index 827e4828f..e289d8925 100644 --- a/include/behaviortree_cpp/utils/safe_any.hpp +++ b/include/behaviortree_cpp/utils/safe_any.hpp @@ -265,8 +265,7 @@ inline bool ValidCast(const SRC& val) { return false; } - SRC back_conv = static_cast(as_float); - return back_conv == val; + // the round-trip check below completes the test } } // Handle conversion to integral diff --git a/src/basic_types.cpp b/src/basic_types.cpp index 9510e5994..2df6bc658 100644 --- a/src/basic_types.cpp +++ b/src/basic_types.cpp @@ -166,7 +166,7 @@ T ConvertWithBoundCheck(StringView str) throw RuntimeError( StrCat("Value out of bound when converting [", str, "] to integer")); } - return res; + return static_cast(res); } } // namespace diff --git a/src/loggers/abstract_logger.cpp b/src/loggers/abstract_logger.cpp index 899a36c24..a20b1aabf 100644 --- a/src/loggers/abstract_logger.cpp +++ b/src/loggers/abstract_logger.cpp @@ -90,7 +90,7 @@ void StatusChangeLogger::handleStatusChange(TimePoint timestamp, const TreeNode& // Copy state under lock, then release before calling user code // This prevents recursive mutex locking when multiple nodes change status bool should_callback = false; - Duration adjusted_timestamp; + Duration adjusted_timestamp{}; { std::unique_lock lk(_p->callback_mutex); if(_p->enabled && (status != NodeStatus::IDLE || _p->show_transition_to_idle)) diff --git a/src/loggers/bt_file_logger_v2.cpp b/src/loggers/bt_file_logger_v2.cpp index 00ba2c923..a4f8808bf 100644 --- a/src/loggers/bt_file_logger_v2.cpp +++ b/src/loggers/bt_file_logger_v2.cpp @@ -113,7 +113,7 @@ void FileLogger2::callback(Duration timestamp, const TreeNode& node, Transition trans{}; trans.timestamp_usec = uint64_t(ToUsec(timestamp - _p->first_timestamp)); trans.node_uid = node.UID(); - trans.status = static_cast(status); + trans.status = static_cast(status); { const std::scoped_lock lock(_p->queue_mutex); _p->transitions_queue.push_back(trans); diff --git a/src/loggers/bt_sqlite_logger.cpp b/src/loggers/bt_sqlite_logger.cpp index f2ed29b4f..b289c3e47 100644 --- a/src/loggers/bt_sqlite_logger.cpp +++ b/src/loggers/bt_sqlite_logger.cpp @@ -185,7 +185,7 @@ void SqliteLogger::callback(Duration timestamp, const TreeNode& node, const std::scoped_lock lk(queue_mutex_); monotonic_timestamp_ = std::max(monotonic_timestamp_ + 1, tm_usec); - long elapsed_time = 0; + int64_t elapsed_time = 0; if(prev_status == NodeStatus::IDLE && status == NodeStatus::RUNNING) { diff --git a/src/xml_parsing.cpp b/src/xml_parsing.cpp index 751b27475..c7f0f3273 100644 --- a/src/xml_parsing.cpp +++ b/src/xml_parsing.cpp @@ -900,7 +900,7 @@ TreeNode::Ptr XMLParser::PImpl::createNodeFromXML(const XMLElement* element, { port_model.converter()(port_value); } - catch(std::exception& ex) + catch(const std::exception&) { auto msg = StrCat("The port with name \"", port_name, "\" and value \"", port_value, "\" can not be converted to ", diff --git a/tests/gtest_any.cpp b/tests/gtest_any.cpp index 1b44c4947..4ae5b72a7 100644 --- a/tests/gtest_any.cpp +++ b/tests/gtest_any.cpp @@ -15,6 +15,7 @@ #include // std::{from_chars,from_chars_result}, #include #include // std::errc. +#include #include @@ -110,13 +111,13 @@ TEST(Any, Cast) // Signed int. { Any a(42); - EXPECT_ANY_THROW((void)a.cast()); + EXPECT_ANY_THROW(std::ignore = a.cast()); EXPECT_EQ(a.cast(), 42); EXPECT_EQ(a.cast(), 42.0); EXPECT_EQ(a.cast(), "42"); Any b(-43); - EXPECT_ANY_THROW((void)b.cast()); + EXPECT_ANY_THROW(std::ignore = b.cast()); EXPECT_EQ(b.cast(), -43); EXPECT_EQ(b.cast(), -43.0); EXPECT_EQ(b.cast(), "-43"); @@ -139,7 +140,7 @@ TEST(Any, Cast) // unsigned int. { Any a(43u); - EXPECT_ANY_THROW((void)a.cast()); + EXPECT_ANY_THROW(std::ignore = a.cast()); EXPECT_EQ(a.cast(), 43u); EXPECT_EQ(a.cast(), 43); EXPECT_EQ(a.cast(), 43.0); @@ -182,7 +183,7 @@ TEST(Any, Cast) EXPECT_TRUE(b.cast()); EXPECT_EQ(typeid(b.cast()), typeid(bool)); // EXPECT_EQ(b.cast(), 44);? - EXPECT_ANY_THROW((void)b.cast()); + EXPECT_ANY_THROW(std::ignore = b.cast()); EXPECT_EQ(b.cast(), 44.1); #if __cpp_lib_to_chars >= 201611L const std::string b_str = b.cast(); @@ -196,13 +197,13 @@ TEST(Any, Cast) Any c(44.9); EXPECT_TRUE(c.cast()); // EXPECT_EQ(c.cast(), 44);? - EXPECT_ANY_THROW((void)c.cast()); + EXPECT_ANY_THROW(std::ignore = c.cast()); EXPECT_EQ(c.cast(), 44.9); Any d(-45.0); EXPECT_TRUE(c.cast()); // EXPECT_EQ(c.cast(), -45);? - EXPECT_ANY_THROW((void)c.cast()); + EXPECT_ANY_THROW(std::ignore = c.cast()); Any e(0.0); EXPECT_FALSE(e.cast()); @@ -212,18 +213,18 @@ TEST(Any, Cast) // String. { Any a(std::string{ "fifty" }); - EXPECT_ANY_THROW((void)a.cast()); - EXPECT_ANY_THROW((void)a.cast()); - EXPECT_ANY_THROW((void)a.cast()); + EXPECT_ANY_THROW(std::ignore = a.cast()); + EXPECT_ANY_THROW(std::ignore = a.cast()); + EXPECT_ANY_THROW(std::ignore = a.cast()); EXPECT_EQ(a.cast(), "fifty"); Any b(std::string{ "true" }); // EXPECT_TRUE(b.cast());? - EXPECT_ANY_THROW((void)b.cast()); + EXPECT_ANY_THROW(std::ignore = b.cast()); Any c(std::string{ "false" }); // EXPECT_FALSE(c.cast());? - EXPECT_ANY_THROW((void)c.cast()); + EXPECT_ANY_THROW(std::ignore = c.cast()); Any d(std::string{ "0" }); // EXPECT_FALSE(d.cast());? @@ -236,12 +237,12 @@ TEST(Any, Cast) EXPECT_EQ(e.cast(), 1.0); Any f(std::string{ "51" }); - EXPECT_ANY_THROW((void)f.cast()); + EXPECT_ANY_THROW(std::ignore = f.cast()); EXPECT_EQ(f.cast(), 51); EXPECT_EQ(f.cast(), 51.0); Any g(std::string{ "51.1" }); - EXPECT_ANY_THROW((void)g.cast()); + EXPECT_ANY_THROW(std::ignore = g.cast()); EXPECT_EQ(g.cast(), 51); EXPECT_DOUBLE_EQ(g.cast(), 51.1); } @@ -267,18 +268,18 @@ TEST(Any, StringToNumberIgnoresLocale) EXPECT_DOUBLE_EQ(Any(std::string("-0.25")).cast(), -0.25); EXPECT_FLOAT_EQ(Any(std::string("1.5e2")).cast(), 150.0f); EXPECT_EQ(Any(std::string("42")).cast(), 42); - EXPECT_ANY_THROW((void)Any(std::string("fifty")).cast()); + EXPECT_ANY_THROW(std::ignore = Any(std::string("fifty")).cast()); } TEST(Any, StringToUint16IsRangeChecked) { // A string that doesn't fit in uint16_t must be rejected, not wrapped. EXPECT_EQ(Any(std::string("65535")).cast(), 65535); - EXPECT_ANY_THROW((void)Any(std::string("65536")).cast()); - EXPECT_ANY_THROW((void)Any(std::string("70000")).cast()); - EXPECT_ANY_THROW((void)Any(std::string("-1")).cast()); + EXPECT_ANY_THROW(std::ignore = Any(std::string("65536")).cast()); + EXPECT_ANY_THROW(std::ignore = Any(std::string("70000")).cast()); + EXPECT_ANY_THROW(std::ignore = Any(std::string("-1")).cast()); auto bb = Blackboard::create(); bb->set("value", std::string("70000")); - EXPECT_ANY_THROW((void)bb->get("value")); + EXPECT_ANY_THROW(std::ignore = bb->get("value")); } diff --git a/tests/gtest_duplicate_tree.cpp b/tests/gtest_duplicate_tree.cpp index ef3798adc..ea3b99ec4 100644 --- a/tests/gtest_duplicate_tree.cpp +++ b/tests/gtest_duplicate_tree.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include @@ -155,8 +156,8 @@ TEST(DuplicateBehaviorTreeId, RepeatedInlineTextIsSilent) BehaviorTreeFactory factory; const std::string xml = minimalTreeXml("MainTree"); const auto output = captureStdout([&] { - (void)factory.createTreeFromText(xml); - (void)factory.createTreeFromText(xml); + std::ignore = factory.createTreeFromText(xml); + std::ignore = factory.createTreeFromText(xml); }); EXPECT_FALSE(hasWarning(output)) << output; } diff --git a/tests/gtest_factory.cpp b/tests/gtest_factory.cpp index 7d4f493a6..cad9312e8 100644 --- a/tests/gtest_factory.cpp +++ b/tests/gtest_factory.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include @@ -366,14 +367,14 @@ TEST(BehaviorTreeFactory, CreateTreeFromFileWhichIncludesItself) { BehaviorTreeFactory factory; std::string path = FilePath("trees/self_include.xml"); - ASSERT_THROW((void)factory.createTreeFromFile(path), RuntimeError); + ASSERT_THROW(std::ignore = factory.createTreeFromFile(path), RuntimeError); } TEST(BehaviorTreeFactory, CreateTreeFromFileWithCyclicIncludes) { BehaviorTreeFactory factory; std::string path = FilePath("trees/parent_include_child_cycle.xml"); - ASSERT_THROW((void)factory.createTreeFromFile(path), RuntimeError); + ASSERT_THROW(std::ignore = factory.createTreeFromFile(path), RuntimeError); // a failed load must not leave stale state behind in the factory's parser Tree tree = factory.createTreeFromFile(FilePath("trees/parent_include_child.xml")); diff --git a/tests/gtest_loggers.cpp b/tests/gtest_loggers.cpp index e3dfef9e5..713e5545e 100644 --- a/tests/gtest_loggers.cpp +++ b/tests/gtest_loggers.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -328,8 +329,9 @@ TEST_F(LoggerTest, MinitraceLogger_JsonContent) std::ifstream file(filepath); ASSERT_TRUE(file.is_open()); - std::string content((std::istreambuf_iterator(file)), - std::istreambuf_iterator()); + std::stringstream buffer; + buffer << file.rdbuf(); + const std::string content = buffer.str(); // Minitrace JSON should contain traceEvents ASSERT_NE(content.find("traceEvents"), std::string::npos); diff --git a/tests/gtest_subtree.cpp b/tests/gtest_subtree.cpp index 30f6da08a..19e211990 100644 --- a/tests/gtest_subtree.cpp +++ b/tests/gtest_subtree.cpp @@ -4,6 +4,7 @@ #include "behaviortree_cpp/xml_parsing.h" #include +#include #include @@ -1344,7 +1345,7 @@ TEST(SubTree, DeclaredPortNameThatCannotBeBound) )"; BehaviorTreeFactory factory; - EXPECT_THROW((void)factory.createTreeFromText(xml_text), RuntimeError); + EXPECT_THROW(std::ignore = factory.createTreeFromText(xml_text), RuntimeError); } // The same underscore attribute on a SubTree that does not declare it is not a @@ -1367,7 +1368,7 @@ TEST(SubTree, UndeclaredUnderscoreAttributeIsNotAPort) )"; BehaviorTreeFactory factory; - EXPECT_NO_THROW((void)factory.createTreeFromText(xml_text)); + EXPECT_NO_THROW(std::ignore = factory.createTreeFromText(xml_text)); } // A declared port nobody remaps stays inert and keeps loading, so existing trees @@ -1390,7 +1391,7 @@ TEST(SubTree, DeclaredUnbindablePortLoadsWhenNotRemapped) )"; BehaviorTreeFactory factory; - EXPECT_NO_THROW((void)factory.createTreeFromText(xml_text)); + EXPECT_NO_THROW(std::ignore = factory.createTreeFromText(xml_text)); } namespace diff --git a/tests/script_parser_test.cpp b/tests/script_parser_test.cpp index 856e35f30..d3cd8715b 100644 --- a/tests/script_parser_test.cpp +++ b/tests/script_parser_test.cpp @@ -3,6 +3,8 @@ #include "behaviortree_cpp/bt_factory.h" #include "behaviortree_cpp/scripting/operators.hpp" +#include + #include #include "../sample_nodes/dummy_nodes.h" @@ -233,7 +235,7 @@ TEST(ParserTest, Equations) EXPECT_EQ(GetResult("y | x").cast(), (5 | 3)); EXPECT_EQ(GetResult("y ^ x").cast(), (5 ^ 3)); - EXPECT_ANY_THROW((void)GetResult("y ^ 5.1").cast()); + EXPECT_ANY_THROW(std::ignore = GetResult("y ^ 5.1").cast()); // test string variables EXPECT_EQ(GetResult("A:='hello'; B:=' '; C:='world'; A+B+C").cast(), "hell" From 40379fc4b47e0ceede0143c5e70740a42afe27cc Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Thu, 12 Nov 2026 20:28:24 +0100 Subject: [PATCH 3/6] Extend expected-lite C4702 suppression to rvalue value(); test anyRef MSVC /W4 reports C4702 in the && overloads of expected::value(); upstream already disables it for the & overloads, so move the restore below them. Co-Authored-By: Claude Opus 5.5 --- include/behaviortree_cpp/contrib/expected.hpp | 3 ++- tests/gtest_json.cpp | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/include/behaviortree_cpp/contrib/expected.hpp b/include/behaviortree_cpp/contrib/expected.hpp index a9a7f172c..d5c5cd6af 100644 --- a/include/behaviortree_cpp/contrib/expected.hpp +++ b/include/behaviortree_cpp/contrib/expected.hpp @@ -2225,7 +2225,6 @@ class expected ? ( contained.value() ) : ( error_traits::rethrow( contained.error() ), contained.value() ); } - nsel_RESTORE_MSVC_WARNINGS() #if !nsel_COMPILER_GNUC_VERSION || nsel_COMPILER_GNUC_VERSION >= 490 @@ -2244,6 +2243,8 @@ class expected } #endif + // BT.CPP: restore moved below the && overloads, which hit C4702 too. + nsel_RESTORE_MSVC_WARNINGS() constexpr error_type const & error() const & { diff --git a/tests/gtest_json.cpp b/tests/gtest_json.cpp index 8b6dd6917..323136eef 100644 --- a/tests/gtest_json.cpp +++ b/tests/gtest_json.cpp @@ -2,6 +2,9 @@ #include "behaviortree_cpp/blackboard.h" #include "behaviortree_cpp/json_export.h" +#include +#include + #include //----------- Custom types ---------- @@ -483,3 +486,15 @@ TEST_F(JsonTest, VectorOfCustomTypes) ASSERT_EQ(poses[1].pos, poses_out[1].pos); ASSERT_EQ(poses[1].rot, poses_out[1].rot); } + +// The converters read the stored value through details::anyRef, which must +// throw instead of dereferencing nullptr on an empty or mismatched BT::Any. +TEST(JsonExporterDetails, AnyRefThrowsOnTypeMismatch) +{ + const std::vector vec = { 1, 2, 3 }; + EXPECT_EQ(BT::details::anyRef>(BT::Any(vec)), vec); + EXPECT_THROW(std::ignore = BT::details::anyRef>(BT::Any()), + BT::LogicError); + EXPECT_THROW(std::ignore = BT::details::anyRef>(BT::Any(vec)), + BT::LogicError); +} From eb1618405178046aeb497c1891cbdb804c081ffb Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Thu, 12 Nov 2026 20:34:38 +0100 Subject: [PATCH 4/6] Fix MSVC /W4 warnings in tests and examples; disable C4702 C4702 (unreachable code) fires on instantiation-dependent if-constexpr tails and has no counterpart in GCC/Clang -Wall -Wextra. Co-Authored-By: Claude Opus 5.5 --- CMakeLists.txt | 5 ++++- examples/t10_observer.cpp | 2 +- tests/gtest_blackboard.cpp | 9 +++++++++ tests/gtest_coroutines.cpp | 5 +++-- tests/gtest_factory.cpp | 3 ++- tests/gtest_reactive.cpp | 3 ++- tests/gtest_substitution.cpp | 6 +++--- tests/gtest_switch.cpp | 1 - 8 files changed, 24 insertions(+), 10 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index c0c488185..e72513c47 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -299,7 +299,10 @@ add_library(BT::${BTCPP_LIBRARY} ALIAS ${BTCPP_LIBRARY}) # warning-free (issue #1288). -Wnull-dereference only fires with optimization. # Set after the 3rdparty subdirectories, so vendored code is not affected. if(MSVC) - set(BTCPP_WARNING_FLAGS /W4 /WX /permissive-) + # C4702 (unreachable code) is a code-generation warning that fires on + # instantiation-dependent `if constexpr` tails; GCC/Clang have no + # equivalent in -Wall -Wextra. + set(BTCPP_WARNING_FLAGS /W4 /WX /permissive- /wd4702) else() set(BTCPP_WARNING_FLAGS -Wall -Wextra -Wpedantic -Wnull-dereference -Werror) endif() diff --git a/examples/t10_observer.cpp b/examples/t10_observer.cpp index 449f9bb09..bf546ddb9 100644 --- a/examples/t10_observer.cpp +++ b/examples/t10_observer.cpp @@ -52,7 +52,7 @@ int main() // a certain set of transitions happened as expected BT::TreeObserver observer(tree); - std::map UID_to_path; + std::map UID_to_path; // Print the unique ID and the corresponding human readable path // Path is also expected to be unique. diff --git a/tests/gtest_blackboard.cpp b/tests/gtest_blackboard.cpp index e6893a332..86b4596ac 100644 --- a/tests/gtest_blackboard.cpp +++ b/tests/gtest_blackboard.cpp @@ -337,10 +337,19 @@ TEST(BlackboardTest, AnyPtrLockedSurvivesClear) auto locked = blackboard->getAnyLocked("value"); ASSERT_TRUE(bool(locked)); +#if defined(_MSC_VER) +#pragma warning(push) +#pragma warning(disable : 4996) +#else #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wdeprecated-declarations" +#endif blackboard->clear(); // deprecated, but still under test +#if defined(_MSC_VER) +#pragma warning(pop) +#else #pragma GCC diagnostic pop +#endif ASSERT_TRUE(blackboard->getKeyNames().empty()); ASSERT_EQ(locked.get()->cast(), 42); } diff --git a/tests/gtest_coroutines.cpp b/tests/gtest_coroutines.cpp index 6507e4fe8..d1880f4ac 100644 --- a/tests/gtest_coroutines.cpp +++ b/tests/gtest_coroutines.cpp @@ -126,7 +126,8 @@ TEST(CoroTest, do_action_timeout) // Action takes longer than timeout -> should fail SimpleCoroAction node(LONG_ACTION_DURATION, false, "Action", node_config_); - BT::TimeoutNode timeout("TimeoutAction", TIMEOUT_DURATION.count()); + BT::TimeoutNode timeout("TimeoutAction", + static_cast(TIMEOUT_DURATION.count())); timeout.setChild(&node); @@ -150,7 +151,7 @@ TEST(CoroTest, sequence_child) // First action completes, second gets halted by timeout SimpleCoroAction actionA(MEDIUM_ACTION_DURATION, false, "action_A", node_config_); SimpleCoroAction actionB(MEDIUM_ACTION_DURATION, false, "action_B", node_config_); - BT::TimeoutNode timeout("timeout", SEQUENCE_TIMEOUT.count()); + BT::TimeoutNode timeout("timeout", static_cast(SEQUENCE_TIMEOUT.count())); BT::SequenceNode sequence("sequence"); timeout.setChild(&sequence); diff --git a/tests/gtest_factory.cpp b/tests/gtest_factory.cpp index cad9312e8..570d9db00 100644 --- a/tests/gtest_factory.cpp +++ b/tests/gtest_factory.cpp @@ -115,7 +115,8 @@ TEST(BehaviorTreeFactory, NotRegisteredNode) { BehaviorTreeFactory factory; ASSERT_ANY_THROW((void)factory.createTreeFromText(xml_text)); - ASSERT_ANY_THROW(std::make_shared(factory.createTreeFromText(xml_text))); + ASSERT_ANY_THROW(std::ignore = + std::make_shared(factory.createTreeFromText(xml_text))); } TEST(BehaviorTreeFactory, XMLParsingOrder) diff --git a/tests/gtest_reactive.cpp b/tests/gtest_reactive.cpp index 92cf3d0ac..f689cbebf 100644 --- a/tests/gtest_reactive.cpp +++ b/tests/gtest_reactive.cpp @@ -4,6 +4,7 @@ #include "behaviortree_cpp/loggers/bt_observer.h" #include +#include #include #include @@ -584,7 +585,7 @@ class TickAllChildren : public BT::ControlNode } }; -long countOf(const std::vector& log, const std::string& event) +std::ptrdiff_t countOf(const std::vector& log, const std::string& event) { return std::count(log.begin(), log.end(), event); } diff --git a/tests/gtest_substitution.cpp b/tests/gtest_substitution.cpp index a8d20c2e6..9275ab007 100644 --- a/tests/gtest_substitution.cpp +++ b/tests/gtest_substitution.cpp @@ -142,17 +142,17 @@ TEST(Substitution, ParserRejectsNonIntegerAsyncDelay) // number and casts an out-of-range one to int (undefined behavior). for(const char* delay : { "1e300", "2.5", "5000000000" }) { - const std::string json_text = std::string(R"( + const std::string bad_json = std::string(R"( { "TestNodeConfigs": { "Delayed": { "return_status": "SUCCESS", "async_delay": )") + - delay + R"( } + delay + R"( } }, "SubstitutionRules": { "actionA": "Delayed" } })"; BehaviorTreeFactory factory; - ASSERT_THROW(factory.loadSubstitutionRuleFromJSON(json_text), RuntimeError) << delay; + ASSERT_THROW(factory.loadSubstitutionRuleFromJSON(bad_json), RuntimeError) << delay; } } diff --git a/tests/gtest_switch.cpp b/tests/gtest_switch.cpp index d208261ee..f33e1f10c 100644 --- a/tests/gtest_switch.cpp +++ b/tests/gtest_switch.cpp @@ -32,7 +32,6 @@ struct SwitchTest : testing::Test input.insert(std::make_pair("case_1", "1")); input.insert(std::make_pair("case_2", "42")); - BT::NodeConfig simple_switch_config_; simple_switch_config_.blackboard = bb; simple_switch_config_.input_ports = input; From b9ede6a3799cfa1a889dc5d448b706a40d977412 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Thu, 12 Nov 2026 20:40:25 +0100 Subject: [PATCH 5/6] gtest_try_catch: static constexpr tick count, no lambda capture needed MSVC requires capturing a const int that clang flags as unneeded. Co-Authored-By: Claude Opus 5.5 --- tests/gtest_try_catch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/gtest_try_catch.cpp b/tests/gtest_try_catch.cpp index 62296bc60..5ff26dbe7 100644 --- a/tests/gtest_try_catch.cpp +++ b/tests/gtest_try_catch.cpp @@ -376,7 +376,7 @@ TEST_F(TryCatchTest, AsyncCatchCompletesInsideSequence) // The catch child returns RUNNING for 5 ticks, then SUCCESS. // Verify that the Sequence keeps ticking TryCatch, which keeps // ticking the catch child until it completes. - const int kRunningTicks = 5; + static constexpr int kRunningTicks = 5; int catch_ticks = 0; factory.registerSimpleCondition("AsyncCleanup", [&catch_ticks](BT::TreeNode&) { catch_ticks++; From d7211f2e1a3d5418713f3099e2ed5775f090b65f Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Thu, 12 Nov 2026 21:01:18 +0100 Subject: [PATCH 6/6] Test the deprecated by-value TestNode constructor gtest_parallel no longer uses it (deprecation warnings are now errors), so cover it explicitly. Co-Authored-By: Claude Opus 5.5 --- tests/gtest_substitution.cpp | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/gtest_substitution.cpp b/tests/gtest_substitution.cpp index 9275ab007..92fb6dd9e 100644 --- a/tests/gtest_substitution.cpp +++ b/tests/gtest_substitution.cpp @@ -643,3 +643,25 @@ TEST(Substitution, DestroyTestNodeWithPendingAsyncDelay) ASSERT_EQ(node->executeTick(), NodeStatus::RUNNING); node.reset(); } + +// The constructor taking TestNodeConfig by value is deprecated but still +// public: keep it working (it delegates to the shared_ptr overload). +TEST(Substitution, DeprecatedTestNodeConstructor) +{ + BT::TestNodeConfig test_config; + test_config.return_status = BT::NodeStatus::FAILURE; +#if defined(_MSC_VER) +#pragma warning(push) +#pragma warning(disable : 4996) +#else +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" +#endif + BT::TestNode node("test", BT::NodeConfig{}, test_config); +#if defined(_MSC_VER) +#pragma warning(pop) +#else +#pragma GCC diagnostic pop +#endif + EXPECT_EQ(node.executeTick(), BT::NodeStatus::FAILURE); +}