Enable -Wshadow=local

[maxwbuckley]

Description

Enables -Wshadow=local for RMM's C++ library, tests, and benchmarks.

Scope (narrowed after review feedback)

This PR was originally written against full -Wshadow, which also warns when constructor parameters shadow class members (e.g. the idiomatic ptr{ptr} member-init pattern) and when locals shadow globals or types. Fixing those required Google-style member_ renames and parameter renames that reviewers (rightly) felt made the code worse — see the discussion below.

It now enables -Wshadow=local instead, which only warns when a local variable or parameter shadows another local variable or parameter. That keeps the genuinely dangerous cases while leaving idiomatic member initialization and member/global shadowing untouched:

Shadow type	-Wshadow	-Wshadow=local (this PR)
Ctor param shadows member (ptr{ptr})	warns	silent
Local shadows global/type	warns	silent
Local shadows local/parameter	warns	warns

Remaining fixes

Every change in this PR fixes a warning that -Wshadow=local actually emits with GCC 13:

1. cpp/src/logger.cpp — the IIFE that initializes the static logger_ declared an inner local also named logger_. Referencing the outer variable inside its own initializer would be UB; the warning is genuinely useful here. Renamed the inner local to instance.
2. cpp/benchmarks/replay/replay.cpp — same pattern: inner args shadows the outer args it is initializing. Renamed to parsed_args.
3. cpp/src/mr/{pinned_host,system}_memory_resource.cpp and cpp/tests/mr/resource_ref_conversion_tests.cpp — deallocate lambdas declared a parameter ptr shadowing the enclosing function's ptr parameter. Renamed the lambda parameter to memory.
4. cpp/tests/mr/callback_mr_tests.cpp — callback lambdas declared a local base_mr that shadows the test-scope base_mr, but refers to a different resource (resolved from arg). Renamed to resolved_mr to make the distinction visible.
5. cpp/tests/mr/mr_ref_*_tests.cpp, thrust_allocator_tests.cu — INSTANTIATE_TEST_SUITE_P name-generator lambdas used a parameter info, which shadows a parameter of the same name inside the gtest macro expansion. Renamed to test_info.

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[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: Enterprise

Run ID: f02672be-1863-49dc-a35e-72b20d7c7506

📥 Commits

Reviewing files that changed from the base of the PR and between f6a8a42 and 9937c3c.

📒 Files selected for processing (11)

• cpp/CMakeLists.txt
• cpp/benchmarks/CMakeLists.txt
• cpp/benchmarks/replay/replay.cpp
• cpp/src/logger.cpp
• cpp/tests/CMakeLists.txt
• cpp/tests/mr/mr_ref_cuda_async_tests.cpp
• cpp/tests/mr/mr_ref_cuda_tests.cpp
• cpp/tests/mr/mr_ref_managed_tests.cpp
• cpp/tests/mr/mr_ref_pinned_tests.cpp
• cpp/tests/mr/mr_ref_system_tests.cpp
• cpp/tests/mr/thrust_allocator_tests.cu

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

• cpp/tests/mr/mr_ref_system_tests.cpp
• cpp/tests/mr/mr_ref_pinned_tests.cpp
• cpp/src/logger.cpp

🚧 Files skipped from review as they are similar to previous changes (6)

• cpp/tests/mr/mr_ref_cuda_tests.cpp
• cpp/tests/CMakeLists.txt
• cpp/tests/mr/mr_ref_cuda_async_tests.cpp
• cpp/CMakeLists.txt
• cpp/benchmarks/replay/replay.cpp
• cpp/benchmarks/CMakeLists.txt

---

📝 Walkthrough

Summary by CodeRabbit

• Refactor

  • Strengthened compiler warnings with stricter checking across build configurations
  • Improved code clarity and maintainability through variable naming and implementation updates
  • Updated internal resource handling patterns

• Chores

  • Updated copyright year ranges

Walkthrough

This PR consolidates internal refactoring and build-configuration improvements: compiler warning flags now include -Wshadow=local across library, benchmark, and test CMake files; memory resource callbacks are refactored to dispatch through the passed device_async_resource_ref; logger initialization is simplified; CLI parsing uses a clearer local variable; and test instantiation parameters are standardized from info to test_info with copyright-year updates.

Changes

Compiler warnings and code quality updates

Layer / File(s)	Summary
Compiler warning flag updates cpp/CMakeLists.txt, cpp/benchmarks/CMakeLists.txt, cpp/tests/CMakeLists.txt	Add -Wshadow=local to C++ and CUDA warning sets across main library, benchmarks, and tests; restructure -Xcompiler lists for consistency.
Memory resource callback and deallocate refactoring cpp/tests/mr/callback_mr_tests.cpp, cpp/src/mr/pinned_host_memory_resource.cpp, cpp/src/mr/system_memory_resource.cpp, cpp/tests/mr/resource_ref_conversion_tests.cpp	Callbacks resolve and dispatch through rmm::device_async_resource_ref passed in arg parameter; deallocate lambda parameters renamed from ptr to memory for semantic clarity.
Logger static initialization refactoring cpp/src/logger.cpp	Refactor default_logger() to construct and configure rapids_logger::logger inline within the static lambda before returning.
Replay benchmark CLI parsing refactoring cpp/benchmarks/replay/replay.cpp	Use explicit local parsed_args variable in CLI parsing lambda to avoid scope collision with outer args.
Test instantiation parameter standardization cpp/tests/mr/mr_ref_arena_tests.cpp, cpp/tests/mr/mr_ref_cuda_async_tests.cpp, cpp/tests/mr/mr_ref_cuda_tests.cpp, cpp/tests/mr/mr_ref_managed_tests.cpp, cpp/tests/mr/mr_ref_pinned_tests.cpp, cpp/tests/mr/mr_ref_system_tests.cpp, cpp/tests/mr/thrust_allocator_tests.cu	Rename GoogleTest instantiation lambda parameter from info to test_info across parameterized test suites; update file copyright years to 2026.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• rapidsai/rmm#2416: Refactored small lambda-parameter renames in cpp/src/mr/pinned_host_memory_resource.cpp and cpp/src/mr/system_memory_resource.cpp directly affect code moved by this PR.
• rapidsai/rmm#2361: Both PRs modify callback invocation patterns in cpp/tests/mr/callback_mr_tests.cpp to use rmm::device_async_resource_ref parameter from arg instead of outer-scope capture.

Suggested labels

non-breaking, improvement

Suggested reviewers

• bdice
• harrism
• wence-

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

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

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Enable -Wshadow=local' directly and accurately describes the primary change: enabling the -Wshadow=local compiler warning flag for the RMM library.
Description check	✅ Passed	The description provides comprehensive context explaining the narrowed scope to -Wshadow=local, justifies each code change, includes a clear summary table, and explains the technical rationale for the switch from full -Wshadow.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ 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.}

[coderabbitai]

🧹 Nitpick comments (1)

cpp/benchmarks/random_allocations/random_allocations.cpp (1)

222-223: ⚡ Quick win

Consider eliminating global state with lambda capture.

While the NOLINT acknowledges the global variables, introducing mutable file-scope state couples benchmark_range to global state and makes testing harder. Google Benchmark supports lambda capture for Apply(), allowing you to avoid globals entirely.

♻️ Proposed refactor using lambda capture

Remove the globals and capture values in the lambda passed to Apply():

-int g_num_allocations = -1;  // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)
-int g_max_size        = -1;  // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)
-
-void benchmark_range(benchmark::Benchmark* bench)
-{
-  if (g_num_allocations > 0) {
-    if (g_max_size > 0) {
-      bench->Args({g_num_allocations, g_max_size})->Unit(benchmark::kMillisecond);
+void benchmark_range(benchmark::Benchmark* bench, int num_allocations, int max_size)
+{
+  if (num_allocations > 0) {
+    if (max_size > 0) {
+      bench->Args({num_allocations, max_size})->Unit(benchmark::kMillisecond);
     } else {
-      size_range(bench, g_num_allocations);
+      size_range(bench, num_allocations);
     }
   } else {
-    if (g_max_size > 0) {
-      num_range(bench, g_max_size);
+    if (max_size > 0) {
+      num_range(bench, max_size);
     } else {
       num_size_range(bench);
     }

Then in declare_benchmark, capture the values:

 void declare_benchmark(std::string const& name)
 {
+  auto apply_range = [num=g_num_allocations, size=g_max_size](benchmark::Benchmark* b) {
+    benchmark_range(b, num, size);
+  };
   if (name == "cuda") {
     BENCHMARK_CAPTURE(BM_RandomAllocations, cuda_mr, &make_cuda)  // NOLINT
-      ->Apply(benchmark_range);
+      ->Apply(apply_range);

Or if you prefer, define the lambda inline in each Apply() call.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cpp/benchmarks/random_allocations/random_allocations.cpp` around lines 222 -
223, Remove the file-scope mutable globals g_num_allocations and g_max_size and
change the benchmarks to capture those values via lambdas passed to Apply();
specifically, stop using the global variables inside benchmark_range and instead
pass the desired num_allocations and max_size into the lambda you hand to
Apply(), then call/forward those captured values into
declare_benchmark/benchmark_range so the state is local to each benchmark
instance.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@cpp/benchmarks/random_allocations/random_allocations.cpp`:
- Around line 222-223: Remove the file-scope mutable globals g_num_allocations
and g_max_size and change the benchmarks to capture those values via lambdas
passed to Apply(); specifically, stop using the global variables inside
benchmark_range and instead pass the desired num_allocations and max_size into
the lambda you hand to Apply(), then call/forward those captured values into
declare_benchmark/benchmark_range so the state is local to each benchmark
instance.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 26bce182-5bd9-4402-ac66-fa866f2b6f31

📥 Commits

Reviewing files that changed from the base of the PR and between 5fd6cee and 08cef2a.

📒 Files selected for processing (39)

• 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/exec_policy.hpp
• cpp/include/rmm/mr/detail/coalescing_free_list.hpp
• cpp/include/rmm/mr/detail/free_list.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/src/mr/detail/arena_memory_resource_impl.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/cccl_mr_ref_test_mt.hpp
• cpp/tests/mr/host_mr_ref_tests.cpp
• cpp/tests/mr/mr_ref_arena_tests.cpp
• cpp/tests/mr/mr_ref_callback_tests.cpp
• cpp/tests/mr/mr_ref_cuda_async_tests.cpp
• cpp/tests/mr/mr_ref_cuda_tests.cpp
• cpp/tests/mr/mr_ref_managed_tests.cpp
• cpp/tests/mr/mr_ref_pinned_tests.cpp
• cpp/tests/mr/mr_ref_system_tests.cpp
• cpp/tests/mr/mr_ref_test.hpp
• 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

[wence-]

While I am broadly positive to this change, please back out all of the renamings that seem to have nothing to do with -Wshadow (e.g. renaming a parameter to a free function from void *ptr to void *raw_pointer etc...)

[wence-]

why?

[wence-]

why?

[wence-]

FWIW, as is the case with most style guides, this is tremendously hateful.

[bdice]

@wence- Can you clarify what you mean here? Are you saying you dislike the idea of suffixing member variables with an underscore as a general practice? I don't mind that rule, and I find it somewhat helpful for reasoning about member state.

[wence-]

I am ok with us deciding that all struct members should have (say) trailing underscore names.

But doing single point rewrites to pacify a warning that I broadly think is just not worth it makes the code worse.

I note as well that the ctor body here is no longer implemented correctly (it refers to stream, not stream_) so this warning didn't help us anyway

[bdice]

Thanks, that is helpful. I agree this will need some polish and close verification to get right.

[wence-]

why?

[wence-]

why?

[wence-]

why?

[wence-]

Oh, I see, -Wshadow is fundamentally broken. This lambda captures no closure but somehow the compiler warns that the name shadows a local.

[wence-]

why?

[wence-]

Having looked harder, I am very negative on this change. -Wshadow seems to be terrifically badly implemented by compilers, providing warnings for completely legitimate code where there is no ambiguity at all.

e.g. in this example:

template<typename Fn>
void do_foo(void *ptr, Fn fn) { fn(ptr); }

int main(void)
{
  void *ptr = nullptr;
  do_foo(ptr, [](void *ptr) { (void)ptr; });
}

The lambda has no default capture, so there is no shadowing. Yet -Wshadow complains. ptr is the right name for this parameter, so I do not want to jump through hoops to pacify a compiler that is badly implemented.

[bdice]

I'm supportive of underscore-suffixed members but there are a lot of unrelated renames happening here. I would echo @wence- that some work-splitting is needed to only make necessary changes. Perhaps we can isolate underscore-suffixed members as a standalone change, and follow that up with further analysis of what is needed for -Wshadow.

I think @wence-'s example of the lambda capture is a difficult case. Per https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55357, this is an intentional behavior in GCC (not necessarily a poor implementation). I am sympathetic to the viewpoint expressed here:

I think it's appropriate to warn here; whether or not there is a default capture seems like a subtle distinction that is easy to miss when reading the code.

I think doing this in pieces will make it easier to review, discuss, and merge.

[bdice]

I reviewed the diff for -Wshadow=local. As I've considered this some more, I am not really convinced that we want to make this change. I don't think the proposed changes are compelling (the name changes are subjectively worse in many of the cases). @maxwbuckley Do you have any reason why this -Wshadow flag is needed, or is it just an attempt to follow best practices? I don't want to draw this out any further if we don't have a strong reason to make the change.

[maxwbuckley]

Thanks for taking another look, @bdice — that's a fair question, and honestly the answer is "best practices" rather than a concrete need. There was no specific bug driving this; it was an attempt to catch a class of shadowing mistakes proactively.

The one place it earned its keep was logger.cpp, where the static logger_'s initializer declared an inner local also named logger_ (referencing the outer one there would be UB). If that's worth fixing I can open a small standalone PR for just that.

Given that's the only compelling case and you and @wence- both feel the renames are subjectively worse, I agree it's not worth the churn. Closing this out — thanks both for the reviews.