Use RMM_EXPORT_NAMESPACE and re-forward MR methods to avoid inlined impl symbols.#2392
Use RMM_EXPORT_NAMESPACE and re-forward MR methods to avoid inlined impl symbols.#2392bdice wants to merge 1 commit into
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR refactors RMM's namespace export strategy and expands memory resource APIs. It introduces ChangesExport namespace refactor and memory resource API expansion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/mr/tracking_resource_adaptor.hpp (1)
28-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the thread-safety contract on the class.
The new stream-aware entry points widen the concurrency surface, but the class Doxygen still does not say which operations are safe to call concurrently. Please spell that out on the type so callers know whether allocation, inspection, and logging APIs can be mixed across threads.
As per coding guidelines,
Document thread-safety guarantees in memory resource class documentation (Doxygen); specify which operations are thread-safe.
🧹 Nitpick comments (3)
cpp/include/rmm/mr/callback_memory_resource.hpp (1)
100-144: ⚡ Quick winClarify how non-default alignment requests are handled.
These new overloads expose an
alignmentparameter, but the visible callback contract above still only carries(bytes, stream, arg). Please either document howalignment > CUDA_ALLOCATION_ALIGNMENTis satisfied here or explicitly reject unsupported alignments in the forwarding implementation so callback authors do not have to guess. As per coding guidelines, "Validate input parameters: check for null pointers, invalid allocation sizes, and invalid alignment values before processing."🤖 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/include/rmm/mr/callback_memory_resource.hpp` around lines 100 - 144, The new overloads allocate, allocate_sync, deallocate, and deallocate_sync must validate requested alignment values and explicitly reject unsupported alignments instead of leaving callback authors guessing: in allocate and allocate_sync check that alignment is a power-of-two and >= rmm::CUDA_ALLOCATION_ALIGNMENT (e.g., (alignment & (alignment-1))==0 and alignment >= rmm::CUDA_ALLOCATION_ALIGNMENT) and throw std::invalid_argument with a clear message if validation fails; for the noexcept deallocate and deallocate_sync perform the same validation but handle failures without throwing (use an assert or log + std::terminate/abort) to fail fast, and ensure the forwarding code that calls the user callback documents that only alignments meeting these conditions are supported.cpp/include/rmm/mr/fixed_size_memory_resource.hpp (1)
71-135: ⚡ Quick winDocument the concurrency contract for the new allocation APIs.
fixed_size_memory_resourcenow exposes four public allocation entry points, but the header still does not state whether concurrentallocate*/deallocate*calls on shared copies are supported. Please add an explicit thread-safety note at the class/API level before shipping this public surface.As per coding guidelines, "Document thread-safety guarantees in memory resource class documentation (Doxygen); specify which operations are thread-safe".
🤖 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/include/rmm/mr/fixed_size_memory_resource.hpp` around lines 71 - 135, Add an explicit Doxygen "Thread-safety" paragraph to the fixed_size_memory_resource class documentation (above the class declaration) that clearly states the concurrency contract for the four public allocation entry points: allocate(cuda::stream_ref,...), deallocate(cuda::stream_ref,...), allocate_sync(...), and deallocate_sync(...). Specify whether concurrent calls on the same shared instance (or copies) are safe or require external synchronization, and call out any differences between the stream-based async APIs and the sync APIs (e.g., if allocate/deallocate are thread-safe but require per-stream ordering, or if none are thread-safe and all require a mutex). Reference the class name fixed_size_memory_resource and the method names in the doc text so users can find the guarantee easily.cpp/include/rmm/mr/logging_resource_adaptor.hpp (1)
110-174: ⚡ Quick winDocument whether shared logger/resource access is thread-safe.
With the new public
allocate*/deallocate*entry points, the header still leaves concurrent-use semantics unspecified. Since copies share both the upstream resource and logger state, please spell out whether concurrent calls are supported and any limitations.As per coding guidelines, "Document thread-safety guarantees in memory resource class documentation (Doxygen); specify which operations are thread-safe".
🤖 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/include/rmm/mr/logging_resource_adaptor.hpp` around lines 110 - 174, The public API for logging_resource_adaptor (methods allocate, deallocate, allocate_sync, deallocate_sync and operator==) lacks documentation of concurrent-use/thread-safety semantics; update the Doxygen for the class and each public method to explicitly state whether shared access to the downstream resource and the shared logger is thread-safe or not, describe which operations may be called concurrently (e.g., concurrent allocate/deallocate on different threads), list any required external synchronization (if de/allocations or logger access are not safe concurrently), and document that copies of logging_resource_adaptor share the same upstream resource and logger state so callers must follow the stated thread-safety rules when using copies; ensure the class-level comment names logging_resource_adaptor, the logger member, and the upstream resource to make the guarantee unambiguous.
🤖 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.
Inline comments:
In `@cpp/include/rmm/mr/prefetch_resource_adaptor.hpp`:
- Line 115: The user-declared destructor ~prefetch_resource_adaptor() suppresses
implicit move operations; explicitly declare and default the special members on
the prefetch_resource_adaptor class (declare copy constructor, copy assignment,
move constructor, and move assignment as =default) so the type remains
copyable/movable and still keeps the separate destructor definition in the .cpp;
keep ~prefetch_resource_adaptor() as-is (defined in the implementation file) but
add prefetch_resource_adaptor(const prefetch_resource_adaptor&) = default;
prefetch_resource_adaptor& operator=(const prefetch_resource_adaptor&) =
default; prefetch_resource_adaptor(prefetch_resource_adaptor&&) = default;
prefetch_resource_adaptor& operator=(prefetch_resource_adaptor&&) = default; to
the header near the class declaration.
In `@cpp/include/rmm/mr/thread_safe_resource_adaptor.hpp`:
- Line 123: The user-declared destructor ~thread_safe_resource_adaptor()
suppresses compiler-generated move operations and is missing Doxygen; fix by
adding a brief Doxygen comment for the destructor describing its behavior and
lifetime semantics, and explicitly default the special members on the class
(thread_safe_resource_adaptor(const thread_safe_resource_adaptor&) = default;
thread_safe_resource_adaptor& operator=(const thread_safe_resource_adaptor&) =
default; thread_safe_resource_adaptor(thread_safe_resource_adaptor&&) = default;
thread_safe_resource_adaptor& operator=(thread_safe_resource_adaptor&&) =
default;) so copy/move remain available despite the user-declared destructor.
In `@cpp/include/rmm/mr/tracking_resource_adaptor.hpp`:
- Line 129: Add a Doxygen comment block for the public destructor
~tracking_resource_adaptor() to match cpp/include/rmm/ coding guidelines:
include a brief description (`@brief`) that it destroys the adaptor and releases
any owned resources, mention noexcept behavior (if applicable) and lifecycle
effects (e.g., does not free underlying memory resource), and add `@since` or
`@deprecated` tags if this introduces a behavioral change; place this comment
immediately above the ~tracking_resource_adaptor() declaration so the destructor
is documented as part of the public API.
---
Nitpick comments:
In `@cpp/include/rmm/mr/callback_memory_resource.hpp`:
- Around line 100-144: The new overloads allocate, allocate_sync, deallocate,
and deallocate_sync must validate requested alignment values and explicitly
reject unsupported alignments instead of leaving callback authors guessing: in
allocate and allocate_sync check that alignment is a power-of-two and >=
rmm::CUDA_ALLOCATION_ALIGNMENT (e.g., (alignment & (alignment-1))==0 and
alignment >= rmm::CUDA_ALLOCATION_ALIGNMENT) and throw std::invalid_argument
with a clear message if validation fails; for the noexcept deallocate and
deallocate_sync perform the same validation but handle failures without throwing
(use an assert or log + std::terminate/abort) to fail fast, and ensure the
forwarding code that calls the user callback documents that only alignments
meeting these conditions are supported.
In `@cpp/include/rmm/mr/fixed_size_memory_resource.hpp`:
- Around line 71-135: Add an explicit Doxygen "Thread-safety" paragraph to the
fixed_size_memory_resource class documentation (above the class declaration)
that clearly states the concurrency contract for the four public allocation
entry points: allocate(cuda::stream_ref,...), deallocate(cuda::stream_ref,...),
allocate_sync(...), and deallocate_sync(...). Specify whether concurrent calls
on the same shared instance (or copies) are safe or require external
synchronization, and call out any differences between the stream-based async
APIs and the sync APIs (e.g., if allocate/deallocate are thread-safe but require
per-stream ordering, or if none are thread-safe and all require a mutex).
Reference the class name fixed_size_memory_resource and the method names in the
doc text so users can find the guarantee easily.
In `@cpp/include/rmm/mr/logging_resource_adaptor.hpp`:
- Around line 110-174: The public API for logging_resource_adaptor (methods
allocate, deallocate, allocate_sync, deallocate_sync and operator==) lacks
documentation of concurrent-use/thread-safety semantics; update the Doxygen for
the class and each public method to explicitly state whether shared access to
the downstream resource and the shared logger is thread-safe or not, describe
which operations may be called concurrently (e.g., concurrent
allocate/deallocate on different threads), list any required external
synchronization (if de/allocations or logger access are not safe concurrently),
and document that copies of logging_resource_adaptor share the same upstream
resource and logger state so callers must follow the stated thread-safety rules
when using copies; ensure the class-level comment names
logging_resource_adaptor, the logger member, and the upstream resource to make
the guarantee unambiguous.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4e4d1b18-aa14-40dc-9139-7c0bcc922026
📒 Files selected for processing (58)
cpp/doxygen/Doxyfilecpp/include/rmm/aligned.hppcpp/include/rmm/cuda_device.hppcpp/include/rmm/cuda_stream.hppcpp/include/rmm/cuda_stream_pool.hppcpp/include/rmm/cuda_stream_view.hppcpp/include/rmm/detail/export.hppcpp/include/rmm/device_buffer.hppcpp/include/rmm/device_scalar.hppcpp/include/rmm/device_uvector.hppcpp/include/rmm/device_vector.hppcpp/include/rmm/error.hppcpp/include/rmm/exec_policy.hppcpp/include/rmm/logger.hppcpp/include/rmm/mr/aligned_resource_adaptor.hppcpp/include/rmm/mr/arena_memory_resource.hppcpp/include/rmm/mr/binning_memory_resource.hppcpp/include/rmm/mr/callback_memory_resource.hppcpp/include/rmm/mr/cuda_async_managed_memory_resource.hppcpp/include/rmm/mr/cuda_async_memory_resource.hppcpp/include/rmm/mr/cuda_async_view_memory_resource.hppcpp/include/rmm/mr/cuda_memory_resource.hppcpp/include/rmm/mr/detail/failure_callback_resource_adaptor_impl.hppcpp/include/rmm/mr/failure_callback_resource_adaptor.hppcpp/include/rmm/mr/failure_callback_t.hppcpp/include/rmm/mr/fixed_size_memory_resource.hppcpp/include/rmm/mr/limiting_resource_adaptor.hppcpp/include/rmm/mr/logging_resource_adaptor.hppcpp/include/rmm/mr/managed_memory_resource.hppcpp/include/rmm/mr/per_device_resource.hppcpp/include/rmm/mr/pinned_host_memory_resource.hppcpp/include/rmm/mr/polymorphic_allocator.hppcpp/include/rmm/mr/pool_memory_resource.hppcpp/include/rmm/mr/prefetch_resource_adaptor.hppcpp/include/rmm/mr/sam_headroom_memory_resource.hppcpp/include/rmm/mr/statistics_resource_adaptor.hppcpp/include/rmm/mr/system_memory_resource.hppcpp/include/rmm/mr/thread_safe_resource_adaptor.hppcpp/include/rmm/mr/thrust_allocator_adaptor.hppcpp/include/rmm/mr/tracking_resource_adaptor.hppcpp/include/rmm/prefetch.hppcpp/include/rmm/process_is_exiting.hppcpp/include/rmm/resource_ref.hppcpp/src/mr/aligned_resource_adaptor.cppcpp/src/mr/arena_memory_resource.cppcpp/src/mr/binning_memory_resource.cppcpp/src/mr/callback_memory_resource.cppcpp/src/mr/cuda_async_managed_memory_resource.cppcpp/src/mr/cuda_async_memory_resource.cppcpp/src/mr/fixed_size_memory_resource.cppcpp/src/mr/limiting_resource_adaptor.cppcpp/src/mr/logging_resource_adaptor.cppcpp/src/mr/pool_memory_resource.cppcpp/src/mr/prefetch_resource_adaptor.cppcpp/src/mr/sam_headroom_memory_resource.cppcpp/src/mr/statistics_resource_adaptor.cppcpp/src/mr/thread_safe_resource_adaptor.cppcpp/src/mr/tracking_resource_adaptor.cpp
| return !(*this == other); | ||
| } | ||
|
|
||
| ~prefetch_resource_adaptor(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -type f -name "prefetch_resource_adaptor.hpp" | head -5Repository: rapidsai/rmm
Length of output: 227
🏁 Script executed:
cat -n cpp/include/rmm/mr/prefetch_resource_adaptor.hppRepository: rapidsai/rmm
Length of output: 5209
🏁 Script executed:
fd -type f -name "shared_resource*" | head -10Repository: rapidsai/rmm
Length of output: 227
🏁 Script executed:
grep -r "shared_resource" cpp/include/rmm/mr/ -A 3 -B 3 | head -50Repository: rapidsai/rmm
Length of output: 3800
🏁 Script executed:
# Check if there are any tests or usage examples for prefetch_resource_adaptor
fd -type f -name "*prefetch*" | grep -E "\.(cpp|cu|hpp|h)$"Repository: rapidsai/rmm
Length of output: 227
🏁 Script executed:
find . -name "*prefetch*" -type fRepository: rapidsai/rmm
Length of output: 502
🏁 Script executed:
# Check the aligned_resource_adaptor to see how it handles destructors
cat -n cpp/include/rmm/mr/aligned_resource_adaptor.hpp | head -130Repository: rapidsai/rmm
Length of output: 5790
🏁 Script executed:
# Look for destructor declarations in similar adaptor classes
grep -n "~" cpp/include/rmm/mr/aligned_resource_adaptor.hpp
grep -n "~" cpp/include/rmm/mr/callback_memory_resource.hpp
grep -n "~" cpp/include/rmm/mr/arena_memory_resource.hppRepository: rapidsai/rmm
Length of output: 158
🏁 Script executed:
# Check if aligned_resource_adaptor has explicitly defaulted copy/move members
cat -n cpp/include/rmm/mr/aligned_resource_adaptor.hpp | sed -n '30,135p'Repository: rapidsai/rmm
Length of output: 4922
🏁 Script executed:
# Check if callback_memory_resource has explicitly defaulted copy/move members
cat -n cpp/include/rmm/mr/callback_memory_resource.hpp | sed -n '30,170p'Repository: rapidsai/rmm
Length of output: 6764
🏁 Script executed:
# Check the implementation file to understand the destructor
cat -n cpp/src/mr/prefetch_resource_adaptor.cppRepository: rapidsai/rmm
Length of output: 2324
🏁 Script executed:
# Check if tests involve moving prefetch_resource_adaptor objects
grep -n "std::move\|&&" cpp/tests/mr/prefetch_resource_adaptor_tests.cpp | head -20Repository: rapidsai/rmm
Length of output: 38
🏁 Script executed:
# Check the aligned_resource_adaptor implementation for comparison
cat -n cpp/src/mr/detail/aligned_resource_adaptor_impl.cpp | head -30Repository: rapidsai/rmm
Length of output: 1185
🏁 Script executed:
# Look for how cuda::mr::shared_resource is used in documentation or comments
grep -r "shared_resource" cpp/include/rmm/ -B 2 -A 2 | grep -E "(move|Move|default)" | head -10Repository: rapidsai/rmm
Length of output: 178
🏁 Script executed:
# Check if arena_memory_resource follows the same pattern
cat -n cpp/include/rmm/mr/arena_memory_resource.hpp | sed -n '140,160p'Repository: rapidsai/rmm
Length of output: 916
🏁 Script executed:
# Check if shared_resource is documented or if we can see its definition
find . -name "*.hpp" -o -name "*.h" | xargs grep -l "class shared_resource" | head -5Repository: rapidsai/rmm
Length of output: 38
🏁 Script executed:
# Look at how other resource adaptors that are already in the codebase handle this
# Check failure_callback_resource_adaptor which might be newer
cat -n cpp/include/rmm/mr/failure_callback_resource_adaptor.hpp | sed -n '1,150p'Repository: rapidsai/rmm
Length of output: 3872
🏁 Script executed:
# Check if there are move operations being tested
grep -r "move.*prefetch_resource_adaptor\|prefetch_resource_adaptor.*move" cpp/tests/ 2>/dev/nullRepository: rapidsai/rmm
Length of output: 38
🏁 Script executed:
# Check if cuda::mr::shared_resource provides move semantics or if it's documented
grep -r "cuda::mr::shared_resource" cpp/include/rmm/ -A 5 -B 2 | grep -E "(move|Move|default)" | head -20Repository: rapidsai/rmm
Length of output: 180
🏁 Script executed:
# Verify the pattern across all resource adaptors - check all those with destructors
echo "=== aligned_resource_adaptor ===" && grep -A 5 -B 5 "~aligned" cpp/include/rmm/mr/aligned_resource_adaptor.hpp | tail -15
echo ""
echo "=== arena_memory_resource ===" && grep -A 5 -B 5 "~arena" cpp/include/rmm/mr/arena_memory_resource.hpp | tail -15
echo ""
echo "=== callback_memory_resource ===" && grep -A 5 -B 5 "~callback" cpp/include/rmm/mr/callback_memory_resource.hpp | tail -15
echo ""
echo "=== prefetch_resource_adaptor ===" && grep -A 5 -B 5 "~prefetch" cpp/include/rmm/mr/prefetch_resource_adaptor.hpp | tail -15Repository: rapidsai/rmm
Length of output: 1476
🏁 Script executed:
# Check if failure_callback_resource_adaptor (which defaults the destructor) has any explicit move declarations
grep -n "move\|Move\|&&\|= default\|= delete" cpp/include/rmm/mr/failure_callback_resource_adaptor.hppRepository: rapidsai/rmm
Length of output: 177
🏁 Script executed:
# Check if there's a pattern in newer code (2024-2026 files) vs older ones
git log --name-only --oneline -- cpp/include/rmm/mr/prefetch_resource_adaptor.hpp cpp/include/rmm/mr/failure_callback_resource_adaptor.hpp 2>/dev/null | head -20Repository: rapidsai/rmm
Length of output: 253
🏁 Script executed:
# Check the actual C++ compiler behavior - test if moves are actually suppressed
cat > /tmp/test_moves.cpp << 'EOF'
`#include` <memory>
// Simulating cuda::mr::shared_resource behavior
template <typename T>
class shared_resource {
public:
shared_resource() = default;
// Implicit copy/move (would be here if no destructor)
};
// Pattern 1: User-declared destructor (like prefetch_resource_adaptor)
class WithUserDtor : public shared_resource<int> {
public:
~WithUserDtor();
};
// Pattern 2: Defaulted destructor (like failure_callback_resource_adaptor)
class WithDefaultDtor : public shared_resource<int> {
public:
~WithDefaultDtor() = default;
};
int main() {
// Test move semantics
static_assert(!std::is_move_constructible_v<WithUserDtor>,
"WithUserDtor should NOT be move constructible");
static_assert(std::is_move_constructible_v<WithDefaultDtor>,
"WithDefaultDtor should be move constructible");
return 0;
}
EOF
# Note: We can't compile, but the logic is clear
cat /tmp/test_moves.cppRepository: rapidsai/rmm
Length of output: 906
🏁 Script executed:
# Check if prefetch_resource_adaptor is actually being moved anywhere in the codebase
rg "prefetch_resource_adaptor" cpp/ -A 3 -B 3 | grep -E "(move|&&|\[\]|return.*prefetch)" | head -20Repository: rapidsai/rmm
Length of output: 595
🏁 Script executed:
# Check the documentation at the top of prefetch_resource_adaptor.hpp to confirm it's a handle type
cat -n cpp/include/rmm/mr/prefetch_resource_adaptor.hpp | sed -n '19,32p'Repository: rapidsai/rmm
Length of output: 603
🏁 Script executed:
# Check if there are any recent commits or PRs that discuss this pattern
git log --all --oneline --grep="destructor\|special member\|move" -- cpp/include/rmm/mr/ 2>/dev/null | head -10Repository: rapidsai/rmm
Length of output: 888
🏁 Script executed:
# Verify what cuda::mr::shared_resource actually provides
# Look for any documentation or usage patterns
grep -r "shared_resource" cpp/ --include="*.hpp" -B 5 -A 2 | grep -E "(template|class|move|Move)" | head -20Repository: rapidsai/rmm
Length of output: 2323
🏁 Script executed:
# Check the tests to see if prefetch_resource_adaptor is ever used in contexts requiring moves
cat cpp/tests/mr/prefetch_resource_adaptor_tests.cpp | head -100Repository: rapidsai/rmm
Length of output: 3439
🏁 Script executed:
cat -n cpp/include/rmm/mr/detail/prefetch_resource_adaptor_impl.hppRepository: rapidsai/rmm
Length of output: 3015
🏁 Script executed:
# Look for any GitHub issues or discussions about this pattern
git log --all -p -- cpp/include/rmm/mr/prefetch_resource_adaptor.hpp | head -200Repository: rapidsai/rmm
Length of output: 7665
🏁 Script executed:
# Check if the prefetch_resource_adaptor tests use copies or moves
grep -n "copy\|move\|=" cpp/tests/mr/prefetch_resource_adaptor_tests.cpp | head -20Repository: rapidsai/rmm
Length of output: 313
🏁 Script executed:
# See if there's an explicit pattern in how failure_callback differs
git show 514a8a49:cpp/include/rmm/mr/failure_callback_resource_adaptor.hpp | grep -A 5 -B 5 "~failure" || echo "Not found in that commit"Repository: rapidsai/rmm
Length of output: 431
🏁 Script executed:
# Check the actual commit to see if the destructor change was intentional
git show 98b05819 -- cpp/include/rmm/mr/prefetch_resource_adaptor.hpp | grep -A 2 -B 2 "~prefetch"Repository: rapidsai/rmm
Length of output: 204
🏁 Script executed:
# Check if other adaptors went through the same change
git show 98b05819 -- cpp/include/rmm/mr/aligned_resource_adaptor.hpp | grep -A 2 -B 2 "~aligned"Repository: rapidsai/rmm
Length of output: 38
🏁 Script executed:
# See what's currently in the main branch for aligned_resource_adaptor destructor
grep -A 2 -B 2 "~aligned_resource_adaptor" cpp/include/rmm/mr/aligned_resource_adaptor.hppRepository: rapidsai/rmm
Length of output: 99
🏁 Script executed:
# Check when the destructor was changed from = default to just declaration
git log -p --all -- cpp/include/rmm/mr/prefetch_resource_adaptor.hpp | grep -A 5 -B 5 "~prefetch" | head -50Repository: rapidsai/rmm
Length of output: 38
🏁 Script executed:
# Check the current git status to see if this file has uncommitted changes
git log --oneline -5 -- cpp/include/rmm/mr/prefetch_resource_adaptor.hppRepository: rapidsai/rmm
Length of output: 147
🏁 Script executed:
# Check what's different between commit 98b05819 and HEAD for this file
git diff 98b05819 HEAD -- cpp/include/rmm/mr/prefetch_resource_adaptor.hpp | head -100Repository: rapidsai/rmm
Length of output: 3622
🏁 Script executed:
# Verify the pattern across all resource adaptors with user-declared destructors
for file in cpp/include/rmm/mr/{aligned,arena,callback,prefetch}*resource*.hpp; do
echo "=== $(basename $file) ==="
grep -A 1 "~" "$file" | grep -E "(~|=)" | head -1
doneRepository: rapidsai/rmm
Length of output: 324
Explicitly default the copy and move special members to preserve move semantics.
The user-declared destructor ~prefetch_resource_adaptor(); suppresses the compiler-generated move constructor and move assignment operator per C++ rules. This creates an observable API regression for a lightweight handle type that is documented as copyable and shares ownership via cuda::mr::shared_resource. Explicitly defaulting the special members ensures the intended movability is preserved without affecting the destructor's separate definition in the implementation file.
♻️ Suggested fix
+ prefetch_resource_adaptor(prefetch_resource_adaptor const&) = default;
+ prefetch_resource_adaptor(prefetch_resource_adaptor&&) = default;
+ prefetch_resource_adaptor& operator=(prefetch_resource_adaptor const&) = default;
+ prefetch_resource_adaptor& operator=(prefetch_resource_adaptor&&) = default;
~prefetch_resource_adaptor();🤖 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/include/rmm/mr/prefetch_resource_adaptor.hpp` at line 115, The
user-declared destructor ~prefetch_resource_adaptor() suppresses implicit move
operations; explicitly declare and default the special members on the
prefetch_resource_adaptor class (declare copy constructor, copy assignment, move
constructor, and move assignment as =default) so the type remains
copyable/movable and still keeps the separate destructor definition in the .cpp;
keep ~prefetch_resource_adaptor() as-is (defined in the implementation file) but
add prefetch_resource_adaptor(const prefetch_resource_adaptor&) = default;
prefetch_resource_adaptor& operator=(const prefetch_resource_adaptor&) =
default; prefetch_resource_adaptor(prefetch_resource_adaptor&&) = default;
prefetch_resource_adaptor& operator=(prefetch_resource_adaptor&&) = default; to
the header near the class declaration.
| return !(*this == other); | ||
| } | ||
|
|
||
| ~thread_safe_resource_adaptor(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the file
find . -type f -name "thread_safe_resource_adaptor.hpp" | head -5Repository: rapidsai/rmm
Length of output: 110
🏁 Script executed:
# Read the file to see the context around line 123
cat -n cpp/include/rmm/mr/thread_safe_resource_adaptor.hpp | head -130Repository: rapidsai/rmm
Length of output: 5327
🏁 Script executed:
# Check if there's a definition of the destructor elsewhere
rg "thread_safe_resource_adaptor::" cpp/include/rmm/mr/thread_safe_resource_adaptor.hpp | head -20Repository: rapidsai/rmm
Length of output: 38
🏁 Script executed:
# Check the implementation file for the destructor definition
find . -type f -name "*thread_safe_resource_adaptor*" | grep -E '\.(cpp|cc|cxx)$'Repository: rapidsai/rmm
Length of output: 160
🏁 Script executed:
# Check the destructor implementation
cat -n cpp/src/mr/thread_safe_resource_adaptor.cppRepository: rapidsai/rmm
Length of output: 2394
🏁 Script executed:
# Verify the base class to understand move semantics
rg "class shared_resource" --type hpp | head -5Repository: rapidsai/rmm
Length of output: 84
🏁 Script executed:
# Search for shared_resource class definition
rg "class shared_resource" -t cpp -t h | head -10Repository: rapidsai/rmm
Length of output: 38
🏁 Script executed:
# Check the cuda library includes
rg "cuda::mr::shared_resource" cpp/include/rmm/mr/thread_safe_resource_adaptor.hpp -A 2 -B 2Repository: rapidsai/rmm
Length of output: 398
🏁 Script executed:
# Check if other public methods in this header have Doxygen comments
rg "^\s*\*\*" cpp/include/rmm/mr/thread_safe_resource_adaptor.hpp | head -20Repository: rapidsai/rmm
Length of output: 38
🏁 Script executed:
# Check the guidelines requirement for public API Doxygen documentation
rg "All public API functions" . -t txt -t mdRepository: rapidsai/rmm
Length of output: 38
🏁 Script executed:
# Verify other public methods in this header have Doxygen
cat -n cpp/include/rmm/mr/thread_safe_resource_adaptor.hpp | grep -E "^\s*(50|60|85|95|106|112|125)" | head -20Repository: rapidsai/rmm
Length of output: 348
Add Doxygen documentation and explicitly default move operations suppressed by the destructor.
The user-declared destructor suppresses the compiler-generated move constructor and move assignment operator per C++11 rules. For this public shared-handle wrapper, that is a quiet API regression. Additionally, the destructor lacks required Doxygen documentation for public API members.
Explicitly default the copy and move special members, and add documentation to the destructor:
♻️ Suggested fix
+ thread_safe_resource_adaptor(thread_safe_resource_adaptor const&) = default;
+ thread_safe_resource_adaptor(thread_safe_resource_adaptor&&) = default;
+ thread_safe_resource_adaptor& operator=(thread_safe_resource_adaptor const&) = default;
+ thread_safe_resource_adaptor& operator=(thread_safe_resource_adaptor&&) = default;
+
+ /**
+ * `@brief` Destroy the thread safe resource adaptor.
+ */
~thread_safe_resource_adaptor();🤖 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/include/rmm/mr/thread_safe_resource_adaptor.hpp` at line 123, The
user-declared destructor ~thread_safe_resource_adaptor() suppresses
compiler-generated move operations and is missing Doxygen; fix by adding a brief
Doxygen comment for the destructor describing its behavior and lifetime
semantics, and explicitly default the special members on the class
(thread_safe_resource_adaptor(const thread_safe_resource_adaptor&) = default;
thread_safe_resource_adaptor& operator=(const thread_safe_resource_adaptor&) =
default; thread_safe_resource_adaptor(thread_safe_resource_adaptor&&) = default;
thread_safe_resource_adaptor& operator=(thread_safe_resource_adaptor&&) =
default;) so copy/move remain available despite the user-declared destructor.
| return !(*this == other); | ||
| } | ||
|
|
||
| ~tracking_resource_adaptor(); |
There was a problem hiding this comment.
Add Doxygen for the destructor.
~tracking_resource_adaptor() is now a public API declaration, but it is the only newly added public function here without a Doxygen block.
Suggested doc stub
+ /**
+ * `@brief` Destroy the adaptor.
+ */
~tracking_resource_adaptor();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ~tracking_resource_adaptor(); | |
| /** | |
| * `@brief` Destroy the adaptor. | |
| */ | |
| ~tracking_resource_adaptor(); |
🤖 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/include/rmm/mr/tracking_resource_adaptor.hpp` at line 129, Add a Doxygen
comment block for the public destructor ~tracking_resource_adaptor() to match
cpp/include/rmm/ coding guidelines: include a brief description (`@brief`) that it
destroys the adaptor and releases any owned resources, mention noexcept behavior
(if applicable) and lifecycle effects (e.g., does not free underlying memory
resource), and add `@since` or `@deprecated` tags if this introduces a behavioral
change; place this comment immediately above the ~tracking_resource_adaptor()
declaration so the destructor is documented as part of the public API.
1 participant
Description
This is a possible solution for #2390, option 1.
Checklist