Enable stricter compiler warnings (-Wshadow, -Wnon-virtual-dtor, -Woverloaded-virtual)

[maxwbuckley]

Summary

• Add -Wshadow, -Wnon-virtual-dtor, and -Woverloaded-virtual to compiler warning flags for the library, tests, and benchmarks (enforced as errors via existing -Werror)
• Mark third-party includes (rapids_logger, spdlog) as SYSTEM to suppress upstream header warnings
• Fix all existing violations across 47 files: shadow variable renames, explicit virtual destructor on owning_wrapper, C-style cast replacement, and GTest macro lambda param renames

Some company compilers enable warning as errors by default and this change makes importing and using RMM easier in those cases.

Test plan

• clang-format run on all modified files — no style violations
• Full build passes at 100% (library + tests + benchmarks) with zero warnings
• All 85 tests pass (ctest --output-on-failure)

🤖 Generated with Claude Code

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 42bfcde8-a401-4003-8887-07f929dfd582

📥 Commits

Reviewing files that changed from the base of the PR and between 0059816 and b9091a9.

📒 Files selected for processing (4)

• cpp/benchmarks/replay/replay.cpp
• cpp/benchmarks/synchronization/synchronization.hpp
• cpp/include/rmm/cuda_stream.hpp
• cpp/include/rmm/exec_policy.hpp

✅ Files skipped from review due to trivial changes (4)

• cpp/include/rmm/cuda_stream.hpp
• cpp/benchmarks/synchronization/synchronization.hpp
• cpp/benchmarks/replay/replay.cpp
• cpp/include/rmm/exec_policy.hpp

---

📝 Walkthrough

Summary by CodeRabbit

• Refactor

  • Updated parameter names across multiple public constructors and functions for improved naming consistency

• Style

  • Enhanced compiler warning flags with stricter checks to improve overall code quality

Walkthrough

CMake warning/compile flags were extended and rapids_logger/spdlog includes marked system; numerous parameter and local-variable identifier renames across src, include, benchmarks, and tests; one class destructor declaration added and a macro no-op changed to use static_cast<void>(0).

Changes

Cohort / File(s)	Summary
CMake Build Configuration cpp/CMakeLists.txt, cpp/benchmarks/CMakeLists.txt, cpp/tests/CMakeLists.txt	Added -Wshadow, -Wnon-virtual-dtor, -Woverloaded-virtual to C++/CUDA flags; pass extra warnings through -Xcompiler; mark rapids_logger/spdlog includes as system to suppress upstream warnings.
Benchmarks cpp/benchmarks/..., cpp/benchmarks/random_allocations/random_allocations.cpp, cpp/benchmarks/replay/replay.cpp, cpp/benchmarks/synchronization/...	Renamed several function/constructor parameters and local variables (e.g., num_allocations→alloc_count, max_size→max_alloc_size, ptr→p, stream→strm) with no behavioral changes.
Core Includes / Public API cpp/include/rmm/cuda_stream.hpp, cpp/include/rmm/exec_policy.hpp, cpp/include/rmm/detail/logging_assert.hpp, cpp/include/rmm/detail/runtime_capabilities.hpp	Parameter renames (flags→stream_flags, stream→strm), macro no-op changed to static_cast<void>(0), and cached-variable rename (is_supported→supported).
Memory-resource Headers & Impl cpp/include/rmm/mr/..., cpp/src/cuda_stream.cpp, cpp/src/exec_policy.cpp	Local/reference renames (arena→stream_arena/dealloc_arena, ptr→p, size→sz), adjusted initializers and usages to match renamed identifiers; no logic changes.
owning_wrapper cpp/include/rmm/mr/owning_wrapper.hpp	Added explicit destructor declaration ~owning_wrapper() override = default; (no semantic change).
Logger Implementation cpp/src/logger.cpp	Renamed local static logger variable (logger_→lgr) within default_logger() initialization lambda only.
Tests — variable renames & instantiations cpp/tests/..., cpp/tests/mr/*, cpp/tests/prefetch_tests.cpp, cpp/tests/device_buffer_tests.cu, cpp/tests/mr/thrust_allocator_tests.cu	Widespread local variable and parameter renames for clarity (size→buff_size/alloc_size, pool→local_pool, ptr→p), many INSTANTIATE_TEST_SUITE_P lambdas renamed parameter info→test_info, some [[maybe_unused]] additions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~14 minutes

Possibly related PRs

• Compile RMM with -Wsign-conversion #2308: Overlapping changes to project CMake warning/compile-flag configuration and test/benchmark compile flags.

Suggested labels

non-breaking, improvement

Suggested reviewers

• ttnghia
• lamarrr
• rongou

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 18.46% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title clearly and concisely summarizes the main changes: enabling three stricter compiler warnings and fixing all violations across the codebase.
Description check	✅ Passed	The PR description is comprehensive and directly related to the changeset, detailing what warnings were added, what was fixed, and the test plan validating the changes.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Tip

You can disable the changed files summary in the walkthrough.

Disable the reviews.changed_files_summary setting to disable the changed files summary in the walkthrough.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

cpp/include/rmm/cuda_stream.hpp (1)

60-63: ⚠️ Potential issue | 🟡 Minor

Update Doxygen param name to match the signature.

After renaming to stream_flags, the doc comment still uses @param flags, which makes API docs inconsistent.

📝 Proposed fix

-   * `@param` flags Stream creation flags.
+   * `@param` stream_flags Stream creation flags.

As per coding guidelines, "cpp/include/rmm/**/*.{h,hpp}: C++ public APIs must include Doxygen comments using /** ... */ format."

Also applies to: 66-66

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cpp/include/rmm/cuda_stream.hpp` around lines 60 - 63, The constructor
Doxygen uses an out-of-date param tag (`@param` flags); update the comment for the
CUDA stream constructor to use the correct parameter name (`@param` stream_flags)
to match the function signature (constructor for class cuda_stream / function
name cuda_stream::cuda_stream or similar) and ensure the comment uses the
required /** ... */ Doxygen style; also apply the same fix to the other
occurrence mentioned (the doc at the second occurrence).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cpp/benchmarks/synchronization/synchronization.hpp`:
- Around line 69-71: Update the Doxygen `@param` for the cuda_event_timer
constructor to use the renamed parameter name "strm" instead of "stream"; locate
the declaration/definition of cuda_event_timer(benchmark::State& state, bool
flush_l2_cache, rmm::cuda_stream_view strm = rmm::cuda_stream_default) and
change the `@param` tag to `@param` strm (or rename the parameter back and make docs
consistent) so the documentation matches the actual argument name.

In `@cpp/include/rmm/exec_policy.hpp`:
- Around line 48-49: Update the Doxygen param names to match the constructor
signatures: change any "@param stream" to "@param strm" for the exec_policy
constructors (the explicit exec_policy(cuda_stream_view strm = ...,
device_async_resource_ref mr = ...) overload and the other constructor overload
referenced around the same area) so the public API docs match the actual
parameter name `strm` and avoid stale documentation.

---

Outside diff comments:
In `@cpp/include/rmm/cuda_stream.hpp`:
- Around line 60-63: The constructor Doxygen uses an out-of-date param tag
(`@param` flags); update the comment for the CUDA stream constructor to use the
correct parameter name (`@param` stream_flags) to match the function signature
(constructor for class cuda_stream / function name cuda_stream::cuda_stream or
similar) and ensure the comment uses the required /** ... */ Doxygen style; also
apply the same fix to the other occurrence mentioned (the doc at the second
occurrence).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3def1fde-be46-4e74-b628-073ded4c98c6

📥 Commits

Reviewing files that changed from the base of the PR and between 1bef459 and 0059816.

📒 Files selected for processing (47)

• cpp/CMakeLists.txt
• cpp/benchmarks/CMakeLists.txt
• cpp/benchmarks/random_allocations/random_allocations.cpp
• cpp/benchmarks/replay/replay.cpp
• cpp/benchmarks/synchronization/synchronization.cpp
• cpp/benchmarks/synchronization/synchronization.hpp
• cpp/benchmarks/utilities/log_parser.hpp
• cpp/include/rmm/cuda_stream.hpp
• cpp/include/rmm/detail/logging_assert.hpp
• cpp/include/rmm/detail/runtime_capabilities.hpp
• cpp/include/rmm/exec_policy.hpp
• cpp/include/rmm/mr/arena_memory_resource.hpp
• cpp/include/rmm/mr/detail/coalescing_free_list.hpp
• cpp/include/rmm/mr/detail/free_list.hpp
• cpp/include/rmm/mr/owning_wrapper.hpp
• cpp/include/rmm/mr/pinned_host_memory_resource.hpp
• cpp/include/rmm/mr/system_memory_resource.hpp
• cpp/src/cuda_stream.cpp
• cpp/src/exec_policy.cpp
• cpp/src/logger.cpp
• cpp/tests/CMakeLists.txt
• cpp/tests/cuda_stream_pool_tests.cpp
• cpp/tests/device_buffer_tests.cu
• cpp/tests/mr/aligned_mr_tests.cpp
• cpp/tests/mr/arena_mr_tests.cpp
• cpp/tests/mr/callback_mr_tests.cpp
• cpp/tests/mr/host_mr_ref_tests.cpp
• cpp/tests/mr/hwdecompress_tests.cpp
• cpp/tests/mr/mr_ref_arena_tests.cpp
• cpp/tests/mr/mr_ref_binning_tests.cpp
• cpp/tests/mr/mr_ref_cuda_async_tests.cpp
• cpp/tests/mr/mr_ref_cuda_tests.cpp
• cpp/tests/mr/mr_ref_fixed_size_tests.cpp
• cpp/tests/mr/mr_ref_managed_tests.cpp
• cpp/tests/mr/mr_ref_pinned_pool_tests.cpp
• cpp/tests/mr/mr_ref_pinned_tests.cpp
• cpp/tests/mr/mr_ref_pool_tests.cpp
• cpp/tests/mr/mr_ref_system_tests.cpp
• cpp/tests/mr/mr_ref_test.hpp
• cpp/tests/mr/mr_ref_test_basic.hpp
• cpp/tests/mr/pool_mr_tests.cpp
• cpp/tests/mr/prefetch_resource_adaptor_tests.cpp
• cpp/tests/mr/resource_ref_conversion_tests.cpp
• cpp/tests/mr/statistics_mr_tests.cpp
• cpp/tests/mr/thrust_allocator_tests.cu
• cpp/tests/mr/tracking_mr_tests.cpp
• cpp/tests/prefetch_tests.cpp

[maxwbuckley]

All CodeRabbit review comments have been addressed in commit b9091a9:

• exec_policy.hpp: @param stream → @param strm (both constructors)
• cuda_stream.hpp: @param flags → @param stream_flags
• synchronization.hpp: @param stream → @param strm
• replay.cpp: stale @param args replaced with @param simulated_size + @param events

[bdice]

Hi, thanks for the PR.

1. Is there a specific environment where you’re observing these warnings?
2. How did you select this set of warnings?
3. Some seems fine to enable but I am unsure about -Wshadow. Can you justify this one in particular? We generally avoid the abbreviated names introduced here like strm for a stream or p for a pointer in favor of full words.

RMM typically requires issues that motivate all pull requests so we have an archive of the library design decisions, could you file an issue containing that motivational information including what environments you observed these? I want to be sure RMM can meet requirements of the libraries depending on it, and I am not aware of any libraries requiring these flags that use RMM.

This PR has a large diff. I am currently refactoring a large part of the library and this will be hard to rebase into the staging branch. Once we settle on what changes we want to make, we can work on a PR based on the staging branch, but I may ask you to wait until some large open PRs land to avoid churn. Thanks!

[maxwbuckley]

Also some companies (Google which I recently left) enforce strict compiler settings by default. So all of these issues make it harder to import RMM into third party.

[maxwbuckley]

Thanks for the detailed feedback, Bradley!

Filed #2338 with the motivation. The short version: some organizations (Google, which I recently left) enforce strict compiler settings by default, so these warnings make it harder to import RMM into third-party projects using -Werror.

To answer your questions:

1. I observed these warnings building with GCC 13 and Clang 18 using -Wall -Wextra -Wshadow -Wnon-virtual-dtor -Woverloaded-virtual -Werror.
2. I selected these specific flags because they catch real bug classes (shadowing, missing virtual destructors, accidentally hidden overloads) and are commonly enforced in projects with strict warning policies.
3. On -Wshadow specifically — I understand the preference for full names over abbreviations like strm/p. Happy to rework those renames to use more descriptive alternatives that still avoid the shadowing (e.g. cuda_stream instead of strm). Let me know what naming you'd prefer and I'll update.

Also happy to wait on the staging branch and rebase once your refactor lands — just let me know when it's a good time. Thanks again!

[bdice]

Awesome. Yes, I think a lot of this will change on the staging branch (it's a near-total rewrite). The staging branch should stabilize in about a week or two. The last major change I need for that branch is #2325 and I'm currently working on splitting that PR up into smaller pieces and working out some design issues with CCCL upstream. I'll follow up on this thread once it's ready.

[ttnghia]

I would expect this PR would cause some sort of breaking builds for downstream libraries. Can we switch this to draft mode, test it against downstream libraries first, apply the same set of compiler directive to them first if needed, then merge this one last?

[maxwbuckley]

Why -Wshadow?

Variable shadowing is a well-documented source of subtle bugs in C++ codebases. When a local variable or parameter shares a name with an outer-scope variable, class member, or inherited member, the programmer may accidentally read or write the wrong one — and the compiler will silently accept it.

Concrete example from this PR: In arena_memory_resource.hpp, a local variable arena shadowed the type alias using arena = rmm::mr::detail::arena::arena. In exec_policy.cpp, the constructor parameter stream shadowed the inherited cudaStream_t stream member from Thrust's execution_policy base class. Both compiled fine before this change, but either could silently break if future code in those scopes referenced the wrong name.

What -Wshadow catches:

• Constructor parameters shadowing class members (e.g., block_base(void* ptr) : ptr{ptr} where ptr is both the param and the member)
• Lambda parameters shadowing enclosing-scope variables (e.g., do_deallocate(void* ptr, ...) { ... [](void* ptr) { ... } })
• Local variables shadowing inherited members from base classes (e.g., Thrust's stream member)
• Variables shadowing type aliases or nested type names

Why now? RMM already enforces -Wall -Werror -Wextra -Wsign-conversion. Adding -Wshadow is the natural next step — it's widely adopted in production C++ codebases (LLVM, Chromium, Qt) and catches a class of bugs that the existing flags miss entirely. The fixes required were all straightforward renames with no behavioral changes.