build: add project C++ warning set with opt-in warnings-as-errors - #882
Draft
maxwbuckley wants to merge 1 commit into
Draft
build: add project C++ warning set with opt-in warnings-as-errors#882maxwbuckley wants to merge 1 commit into
maxwbuckley wants to merge 1 commit into
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
9 tasks
Contributor
|
📝 Docs preview is not auto-deployed for fork PRs. A maintainer with write access to |
Adds cmake/CompilerWarnings.cmake, included from the root CMakeLists.txt *after* add_subdirectory(deps). Directory-scope compile options are only inherited by subdirectories added after the call, so first-party targets get the flags while third-party trees (OpenXR SDK, yaml-cpp, pybind11, mcap, flatbuffers, Catch2) keep their own. Flags on GNU/Clang: -Wall -Wextra -Wno-missing-field-initializers -Wnon-virtual-dtor -Woverloaded-virtual -Wimplicit-fallthrough -Wextra-semi. MSVC gets /W4 /permissive-. Two options: ISAAC_TELEOP_ENABLE_WARNINGS (ON) and ISAAC_TELEOP_WARNINGS_AS_ERRORS (OFF, opt in per build/CI). Ordering handles deps/, but not a tree fetched from *inside* an already-flagged directory: a subdirectory snapshots COMPILE_OPTIONS at the point it is added, so what the plugins pull in via FetchContent would inherit our flags. The OAK plugin fetches DepthAI, which fetches XLink, and XLink does not build at -Wall -Wextra -Werror — it sets -Wno-unused-parameter for itself precisely because it does not hold to that. Two mechanisms keep it out: - isaac_teleop_third_party_scope_begin()/_end() clear the calling directory's COMPILE_OPTIONS across the add_subdirectory()/FetchContent_MakeAvailable() and restore them afterwards, so the fetched tree builds with its own flags while first-party targets later in the same file still get ours. Used for DepthAI and SDL2 in the OAK plugin, and nlohmann/json in OGLO. - isaac_teleop_mark_include_dirs_system() moves a dependency's interface includes to -isystem, because a warning raised inside a header is attributed to the first-party TU that included it — DepthAI's headers alone accounted for 128 -Wextra-semi hits in our own sources. (add_subdirectory(... SYSTEM) does this in one step but needs CMake 3.25; the project floor is 3.20.) Fixes everything the set surfaced on first-party code: - properties.serial is a fixed char[256], never a pointer, so the `properties.serial ? ... : ""` guard was always-true dead code (-Wpointer-bool-conversion, 3 sites). Replaced with a strnlen-bounded std::string construction, which additionally guards against a runtime that fills the array without a terminator. - Removed the empty, unused print_xdev_info (-Wunused-function). - Commented out the unused argc parameter name in six main() definitions. - oak_camera.cpp called the deprecated dai::DeviceInfo::getMxId() (-Wdeprecated-declarations); the same file already used getDeviceId() in the two neighbouring call sites. robstride_bus's private members are used only inside #ifdef __linux__, so Clang reports them unused when the file compiles to its throwing stub. That warning is correct but unactionable off Linux (GCC has no equivalent), so it is suppressed for that one target on non-Linux only. Verified on GCC 13.3 / x86_64 (Ubuntu 24.04): clean build of 339 first-party TUs with ISAAC_TELEOP_WARNINGS_AS_ERRORS=ON and zero warnings, including the OGLO and Noitom plugins and the Televiz tree; BUILD_PLUGIN_OAK_CAMERA=ON also builds clean under -Werror, DepthAI and XLink compiling with their own flags throughout. ctest 309/310, the one failure being a missing CloudXR SDK on the host. Not yet validated on MSVC, so CI is deliberately left at the OFF default. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Max Buckley <maxwbuckley@gmail.com>
maxwbuckley
force-pushed
the
chore/cpp-compiler-warnings
branch
from
August 4, 2026 14:12
c5900d8 to
d8725f2
Compare
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.
Description
Adds
cmake/CompilerWarnings.cmake, included from the rootCMakeLists.txtafteradd_subdirectory(deps). Directory-scope compile options are only inherited bysubdirectories added after the call, so first-party targets get the flags while the
third-party trees (OpenXR SDK, yaml-cpp, pybind11, mcap, flatbuffers, Catch2) keep
their own.
Flags on GNU/Clang:
-Wall -Wextra -Wno-missing-field-initializers -Wnon-virtual-dtor -Woverloaded-virtual -Wimplicit-fallthrough -Wextra-semi. MSVC gets/W4 /permissive-.Two options:
ISAAC_TELEOP_ENABLE_WARNINGS(ON) andISAAC_TELEOP_WARNINGS_AS_ERRORS(OFF, opt in per build/CI).
Ordering handles
deps/, but not a tree fetched from inside an already-flaggeddirectory: a subdirectory snapshots
COMPILE_OPTIONSat the point it is added, so whatthe plugins pull in via FetchContent would inherit our flags. The OAK plugin fetches
DepthAI, which fetches XLink, and XLink does not build at
-Wall -Wextra -Werror— itsets
-Wno-unused-parameterfor itself precisely because it does not hold to that. Twomechanisms keep our flags out:
isaac_teleop_third_party_scope_begin()/_end()clear the calling directory'sCOMPILE_OPTIONSacross theadd_subdirectory()/FetchContent_MakeAvailable()andrestore them afterwards, so the fetched tree builds with its own flags while
first-party targets later in the same file still get ours. Used for DepthAI and SDL2
in the OAK plugin, and nlohmann/json in OGLO.
isaac_teleop_mark_include_dirs_system()moves a dependency's interface includes to-isystem, because a warning raised inside a header is attributed to the first-partyTU that included it — DepthAI's headers alone accounted for 128
-Wextra-semihits inour own sources. (
add_subdirectory(... SYSTEM)does this in one step but needs CMake3.25; the project floor is 3.20.)
Fixes everything the set surfaced on first-party code:
properties.serialis a fixedchar[256], never a pointer, so theproperties.serial ? ... : ""guard was always-true dead code(
-Wpointer-bool-conversion, 3 sites). Replaced with astrnlen-boundedstd::stringconstruction, which additionally guards against a runtime that fillsthe array without a terminator.
print_xdev_info(-Wunused-function).argcparameter name in sixmain()definitions.oak_camera.cppcalled the deprecateddai::DeviceInfo::getMxId()(
-Wdeprecated-declarations); the same file already usedgetDeviceId()in the twoneighbouring call sites.
robstride_bus's private members are used only inside#ifdef __linux__, so Clangreports them unused when the file compiles to its throwing stub. That warning is
correct but unactionable off Linux (GCC has no equivalent), so it is suppressed for
that one target on non-Linux only.
C++/CMake only — no Python is touched. A companion PR configures the ruff lint rules
for the Python side; the two are independent and can land in either order.
Type of change
Testing
GCC 13.3 / x86_64, Ubuntu 24.04, RTX PRO 6000 Blackwell, CUDA 13.0 —
-Werror, withBUILD_PLUGIN_OGLO=ONandBUILD_PLUGIN_NOITOM_MOCAP=ON. This includesrebot_devarm_leaderon its real SocketCAN path (so the-Wno-unused-private-fieldsuppression above is confirmed to be needed only off Linux), and the whole Televiz
tree with
BUILD_VIZauto-ON (Vulkan + CUDA + glslang all present).BUILD_PLUGIN_OAK_CAMERA=ON(vcpkg toolchain) also builds clean under-Werror.Verified from the generated
flags.makethat the containment cuts in one directiononly:
camera_plugin_oakgets the full project set including-Werrorand reachesDepthAI's headers via
-isystem, whiledepthai-coreandXLinkcompile with theirown upstream warning flags and none of ours. Before the containment this configuration
failed with 41 errors inside
xlink-src.ctest: 309/310. The one failure,cloudxr_test_launcher, is the missing CloudXR SDK(no NGC key on this host, so the download 404s and
get_sdk_path()raises) — anenvironment gap, not a code failure.
SKIP=check-copyright-year pre-commit run --all-files: all hooks pass.MSVC is unvalidated, so CI is deliberately left at the
ISAAC_TELEOP_WARNINGS_AS_ERRORSOFF default.
Checklist
SKIP=check-copyright-year pre-commit run --all-filesgit commit -s) per the DCODocumentation: no user-facing behaviour changes, so no doc updates. The two new CMake
options are documented in the header comment of
cmake/CompilerWarnings.cmake.Tests: no new tests — this is a build-configuration change, and it is exercised by the
existing suite building and passing under
-Werroron both toolchains above.