Compile RMM with -Wsign-conversion

[Matt711]

Description

Compiles rmm with -Wsign-conversion -Werror and fixes the failures

• Closes [BUG] implicit int -> size_t conversions in rmm headers break downstream builds with -Wsign-conversion -Werror #2307

Checklist

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

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Replaced implicit int→std::size_t conversions with explicit std::size_t/static_casts across headers, tests, and utilities; added -Wsign-conversion to several CMake flag sets; minor formatting and SPDX year updates.

Changes

Cohort / File(s)	Summary
Build flags cpp/CMakeLists.txt, cpp/benchmarks/CMakeLists.txt, cpp/tests/CMakeLists.txt	Add -Wsign-conversion to CXX/CUDA flags; apply per-target compile_options to silence the warning for specific benchmark/test targets.
Core headers (sign-conversion fixes) cpp/include/rmm/detail/format.hpp, cpp/include/rmm/mr/detail/stream_ordered_memory_resource.hpp	Change local index types to std::size_t / add static_cast<std::size_t> for device-count and indexing to remove implicit signed→unsigned conversions; SPDX year update in format.hpp.
Allocators / internals cpp/include/rmm/mr/fixed_size_memory_resource.hpp, cpp/include/rmm/mr/statistics_resource_adaptor.hpp	Change block/index lambda parameter types to std::size_t; cast sizes to int64_t when updating statistics counters.
Bench utilities cpp/benchmarks/utilities/log_parser.hpp	Parse pointer hex strings into uintptr_t via explicit cast (replace direct std::stoll→pointer assignment).
Tests — index/iterator fixes cpp/tests/mr/mr_ref_test.hpp, cpp/tests/mr/mr_ref_test_mt.hpp, cpp/tests/mr/tracking_mr_tests.cpp, cpp/tests/prefetch_tests.cpp	Use std::size_t or explicit casts (std::size_t, std::ptrdiff_t) for indices and iterator arithmetic to silence sign-conversion warnings; small SPDX year updates.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Drop usage of thrust iterators #2254: Changes in cpp/include/rmm/mr/fixed_size_memory_resource.hpp related to block-index handling — closely related to the index-type changes here.

Suggested labels

improvement

Suggested reviewers

• rongou
• harrism

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 30.00% 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 'Compile RMM with -Wsign-conversion' clearly and concisely describes the main objective of the PR.
Description check	✅ Passed	The description explains that the PR compiles RMM with -Wsign-conversion -Werror and fixes failures, directly relating to the changeset.
Linked Issues check	✅ Passed	The PR fixes all sign-conversion issues identified in #2307: detail/format.hpp (int→size_t), stream_ordered_memory_resource.hpp (two locations), and adds -Wsign-conversion flags throughout CMakeLists files to enforce compliance.
Out of Scope Changes check	✅ Passed	All changes in the PR are directly related to addressing sign-conversion warnings: type casts for index/size parameters, CMakeLists enforcement flags, and copyright year updates are all in scope.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

📝 Coding Plan

• Generate coding plan for human review comments

---

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

[coderabbitai]

🧹 Nitpick comments (1)

cpp/include/rmm/detail/safe_cast.hpp (1)

14-30: Documentation claims broader checking than implementation provides.

The docstring states this is a "Checked narrowing/sign-converting cast" that asserts the value is "representable in To", but the implementation only checks for negative values when converting signed→unsigned. Narrowing conversions (e.g., int64_t → int32_t) are not validated and could silently overflow.

For the current PR use cases (widening int → std::size_t), this is sufficient. Consider either:

1. Updating the docstring to reflect the actual scope (sign-converting only), or
2. Adding a narrowing check for completeness.

Option 1: Update documentation to match implementation

 /**
- * `@brief` Checked narrowing/sign-converting cast.
+ * `@brief` Checked sign-converting cast for widening integral conversions.
  *
- * Casts `value` to type `To`, asserting at runtime that the value is
- * representable in `To`. In release builds the assertion compiles away when
+ * Casts `value` to type `To`, asserting at runtime that signed values are
+ * non-negative when converting to unsigned types. In release builds the assertion compiles away when
  * the compiler can prove it is always true.
  */

Option 2: Add narrowing check for full representability

 template <typename To, typename From>
 [[nodiscard]] constexpr To safe_cast(From value)
 {
   static_assert(std::is_integral_v<From> && std::is_integral_v<To>,
                 "safe_cast is only defined for integral types");
   if constexpr (std::is_signed_v<From> && std::is_unsigned_v<To>) {
     RMM_EXPECTS(value >= From{0}, "safe_cast: negative value cannot be represented as unsigned");
   }
+  if constexpr (sizeof(From) > sizeof(To) || 
+                (std::is_signed_v<From> == std::is_signed_v<To> && sizeof(From) == sizeof(To))) {
+    // Potentially narrowing; check bounds
+    RMM_EXPECTS(value <= static_cast<From>(std::numeric_limits<To>::max()) &&
+                value >= static_cast<From>(std::numeric_limits<To>::min()),
+                "safe_cast: value out of range for target type");
+  }
   return static_cast<To>(value);
 }

Note: Option 2 would require adding #include <limits>.

🤖 Prompt for AI Agents

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

In `@cpp/include/rmm/detail/safe_cast.hpp` around lines 14 - 30, The docstring
overstates checks—implement full representability checks in safe_cast instead of
only signed→unsigned negative checks: include <limits> and inside template
safe_cast add constexpr branches that for narrowing conversions (when sizeof(To)
< sizeof(From) or differing signedness) verify value <=
std::numeric_limits<To>::max() and value >= std::numeric_limits<To>::min() as
applicable, using RMM_EXPECTS for failures; retain the existing negative check
for signed→unsigned but expand logic to cover signed→signed, unsigned→unsigned,
and unsigned→signed by comparing against numeric_limits<To>::min()/max()
(convert bounds to From for comparison) so all integral representability is
asserted before static_cast in safe_cast.

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@cpp/include/rmm/detail/safe_cast.hpp`:
- Around line 14-30: The docstring overstates checks—implement full
representability checks in safe_cast instead of only signed→unsigned negative
checks: include <limits> and inside template safe_cast add constexpr branches
that for narrowing conversions (when sizeof(To) < sizeof(From) or differing
signedness) verify value <= std::numeric_limits<To>::max() and value >=
std::numeric_limits<To>::min() as applicable, using RMM_EXPECTS for failures;
retain the existing negative check for signed→unsigned but expand logic to cover
signed→signed, unsigned→unsigned, and unsigned→signed by comparing against
numeric_limits<To>::min()/max() (convert bounds to From for comparison) so all
integral representability is asserted before static_cast in safe_cast.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 160181ea-f492-4399-bdc1-dd581340a724

📥 Commits

Reviewing files that changed from the base of the PR and between 22d812c and d23657c.

📒 Files selected for processing (3)

• cpp/include/rmm/detail/format.hpp
• cpp/include/rmm/detail/safe_cast.hpp
• cpp/include/rmm/mr/detail/stream_ordered_memory_resource.hpp

[bdice]

@Matt711 Please update the PR description and then feel free to trigger a merge.

[bdice]

I almost forgot to ask:

Please update these to enforce this requirement, too:

rmm/cpp/CMakeLists.txt

Lines 127 to 129 in 22d812c

	set(RMM_CXX_FLAGS -Wall -Werror -Wextra -Wno-unknown-pragmas -Wno-error=deprecated-declarations)
	set(RMM_CUDA_FLAGS -Werror=all-warnings
	-Xcompiler=-Wall,-Werror,-Wextra,-Wno-error=deprecated-declarations)

rmm/cpp/tests/CMakeLists.txt

Lines 12 to 13 in 22d812c

	set(RMM_TESTS_CXX_FLAGS -Wall -Werror -Wextra -Wno-unknown-pragmas)
	set(RMM_TESTS_CUDA_FLAGS -Werror=all-warnings -Xcompiler=-Wall,-Werror,-Wextra)

rmm/cpp/benchmarks/CMakeLists.txt

Lines 12 to 13 in 22d812c

	set(RMM_BENCHMARKS_CXX_FLAGS -Wall -Werror -Wextra -Wno-unknown-pragmas)
	set(RMM_BENCHMARKS_CUDA_FLAGS -Werror=all-warnings -Xcompiler=-Wall,-Werror,-Wextra)

[Matt711]

I almost forgot to ask:

Please update these to enforce this requirement, too:

rmm/cpp/tests/CMakeLists.txt

Lines 12 to 13 in 22d812c

	set(RMM_TESTS_CXX_FLAGS -Wall -Werror -Wextra -Wno-unknown-pragmas)
	set(RMM_TESTS_CUDA_FLAGS -Werror=all-warnings -Xcompiler=-Wall,-Werror,-Wextra)

rmm/cpp/benchmarks/CMakeLists.txt

Lines 12 to 13 in 22d812c

	set(RMM_BENCHMARKS_CXX_FLAGS -Wall -Werror -Wextra -Wno-unknown-pragmas)
	set(RMM_BENCHMARKS_CUDA_FLAGS -Werror=all-warnings -Xcompiler=-Wall,-Werror,-Wextra)

Yup working on that now. There are more bugs to fix

[coderabbitai]

Actionable comments posted: 1

🤖 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/utilities/log_parser.hpp`:
- Line 149: The parse_pointer lambda currently uses std::stoll to parse hex
pointer strings into uintptr_t which can overflow for high-bit values; change
the call to std::stoull so unsigned parsing is used (keep the "(nil)" check) and
cast the result to uintptr_t (e.g. replace the std::stoll call with std::stoull
while leaving the ternary and static_cast<uintptr_t> intact) in the
parse_pointer/ptr assignment.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f73316ca-0937-450e-a46a-c5cb11da26d9

📥 Commits

Reviewing files that changed from the base of the PR and between 5e327b5 and d001da1.

📒 Files selected for processing (10)

• cpp/CMakeLists.txt
• cpp/benchmarks/CMakeLists.txt
• cpp/benchmarks/utilities/log_parser.hpp
• cpp/include/rmm/mr/fixed_size_memory_resource.hpp
• cpp/include/rmm/mr/statistics_resource_adaptor.hpp
• cpp/tests/CMakeLists.txt
• cpp/tests/mr/mr_ref_test.hpp
• cpp/tests/mr/mr_ref_test_mt.hpp
• cpp/tests/mr/tracking_mr_tests.cpp
• cpp/tests/prefetch_tests.cpp

[bdice]

This is vendored -- no need to review its diff.

[bdice]

Thanks @Matt711! I got this fixed up with a few more changes in benchmarks. I'm going to merge this once CI passes, so that I can merge main into staging and continue my work on the CCCL MR refactoring.

[bdice]

/merge