Repository navigation
Fix GCC 15 -Wnull-dereference in public headers; always build with -Werror - #1290
Merged
Merged
Conversation
…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<weak_ptr> copy-assignment; copy-construct the snapshot instead. - JsonExporter converters: *castPtr<T>() 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 <noreply@anthropic.com>
facontidavide
marked this pull request as ready for review
November 12, 2026 19:11
- 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
MSVC requires capturing a const int that clang flags as unneeded. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gtest_parallel no longer uses it (deprecation warnings are now errors), so cover it explicitly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1288. Supersedes #1287 (same
ThreadSafeSignalfix, credited as co-author).Problem
Downstream packages (Nav2, ROS Lyrical) compile our public headers with
-Werror. With GCC 15 at-O2,-Wnull-dereferencefires in:utils/thread_safe_signal.h:52(the reported error). This is a GCC false positive instd::vector<std::weak_ptr>copy-assignment.json_export.h(not reported yet).*castPtr<T>()was dereferenced without a null check, andcastPtrreturnsnullptrwhen theAnyis empty or holds another type.Our CI never caught it: the GCC 15 job (
ros2-lyrical) builds without optimization, and tests/examples were never built with-Werror.Fix
ThreadSafeSignal::notify()copy-constructs the subscriber snapshot instead of copy-assigning it.details::anyRef<T>(). Behavior change: an empty or mismatchedAnynow throwsLogicError. Before, it was undefined behavior. A unit test covers the new branch.BT_REGISTER_NODESis now[[maybe_unused]] static, andPLUGIN_SYMBOLis nowinline constexpr.contrib/expected.hpp: upstream's own C4702 suppression is extended to the&&overloads ofvalue().Prevent regressions
The library and every in-tree consumer of its headers (sample_nodes, tests, tools, examples, fuzzers) now always build with:
-Wall -Wextra -Wpedantic -Wnull-dereference -Werror/W4 /WX /permissive- /wd4702. C4702 (unreachable code) is a code-generation warning that fires on instantiation-dependentif constexprtails, and GCC/Clang-Wall -Wextrahas no counterpart.Vendored 3rdparty code is not affected.
ros2-lyrical(GCC 15) now builds inRelease, so-Wnull-dereferenceis active there.Existing warnings in the library, tests, examples and fuzzers are fixed, mostly at the root:
#pragma GCC diagnostic popinxml_parsing.cpp;ConvertWithBoundCheck;StatusChangeLogger;get_if/dynamic_castresults in tests;TestNodeconstructor ingtest_parallel;-Wnull-dereferencefalse positive ingtest_loggers.Discarded
[[nodiscard]]results usestd::ignore =.No ABI change: only inline templates, a macro and a constant are touched in the headers.
Verified
thread_safe_signal.hfix reproduces the exact error from New BT.CPP Build Errors with -Werror #1288.🤖 Generated with Claude Code