Add opc_classic_utils crate for COM memory management utilities#21
Add opc_classic_utils crate for COM memory management utilities#21
Conversation
WalkthroughA new Rust crate, Changes
Sequence Diagram(s)sequenceDiagram
participant Client as OPC Client (Rust)
participant Server as OPC Server (COM)
participant Utils as opc_classic_utils
Client->>Utils: Allocate CallerAllocatedPtr/Array/WString for input
Client->>Server: Call COM method with input pointers
Server->>Utils: Allocate CalleeAllocatedPtr/Array/WString for output
Server-->>Client: Return output pointers (ownership transferred)
Client->>Utils: Use output wrappers (auto free on drop)
sequenceDiagram
participant RustCode as Rust User Code
participant Utils as opc_classic_utils
RustCode->>Utils: Create CallerAllocatedPtr<T> (allocates, does not free on drop)
RustCode->>Utils: Create CalleeAllocatedPtr<T> (takes ownership, frees on drop)
RustCode->>Utils: Create CallerAllocatedArray<T> / CalleeAllocatedArray<T>
RustCode->>Utils: Create CallerAllocatedWString / CalleeAllocatedWString
RustCode->>Utils: Use safe/unsafe accessors, conversions, and pointer transfer
Note right of Utils: Drop implementations handle memory according to ownership pattern
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (3)📓 Common learningsopc_classic_utils/src/memory/ptr.rs (4)opc_classic_utils/src/memory/tests.rs (4)🧬 Code Graph Analysis (1)opc_classic_utils/src/memory/ptr.rs (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (5)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
Cargo.toml (1)
17-28: Inconsistent windows crate family versions will cause duplicate crates
windows = 0.61.3,windows-core = 0.61.2,windows-bindgen = 0.62.1, andwindows-targets = 0.53.2are out of sync.
windowsre-exports a matchingwindows-coreandwindows-targets; the current mix will pull two semver-incompatible copies of each crate and break linker-time type identity.Align them to a single minor version (≥ 0.62 recommended because
windows-bindgenis already 0.62):-windows = { version = "0.61.3", features = [ ... ] } -windows-core = "0.61.2" -windows-bindgen = "0.62.1" -windows-targets = "0.53.2" +windows = { version = "0.62.1", features = [ ... ] } +windows-core = "0.62.1" +windows-bindgen = "0.62.1" +windows-targets = "0.62.1"Cargo will then unify to a single copy of each crate.
🧹 Nitpick comments (1)
Cargo.toml (1)
16-16: Duplicated Tokio feature
"full"already enables"rt-multi-thread". Listing both is harmless but redundant and lengthens feature resolution.-tokio = { version = "1.46.1", features = ["full", "rt-multi-thread"] } +tokio = { version = "1.46.1", features = ["full"] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Cargo.toml(1 hunks)opc_da/Cargo.toml(1 hunks)
🔇 Additional comments (2)
Cargo.toml (1)
12-16: Version + path combo prevents publishing crates
opc_comn_bindingsandopc_da_bindingsare declared with bothversionandpath.
cargo publishrefuses to publish any member that has path deps; the additionalversionspec is therefore redundant and may mis-lead future maintainers into thinking these crates are published on crates.io.-opc_comn_bindings = { version = "0.3.0", path = "opc_comn_bindings" } -opc_da_bindings = { version = "0.3.1", path = "opc_da_bindings" } +opc_comn_bindings = { path = "opc_comn_bindings" } +opc_da_bindings = { path = "opc_da_bindings" }If you intend to publish those sub-crates later, split the workspace or swap to
version =only before publishing.opc_da/Cargo.toml (1)
15-21: Workspace aliases inherit the root-level version mismatchGood move switching to
{ workspace = true }.
However, until the rootCargo.tomlaligns all windows-family versions (see comment above), this crate will still suffer the duplicate-crate problem when compiled.Re-check after fixing the root manifest.
- Add new opc_classic_utils package for shared OPC Classic functionality - Implement dual COM memory management patterns: * CallerAllocatedPtr<T> - caller allocates, callee frees (input params) * CalleeAllocatedPtr<T> - callee allocates, caller frees (output params) * CallerAllocatedWString - caller-allocated wide strings * CalleeAllocatedWString - callee-allocated wide strings - Add #[repr(transparent)] for zero-cost FFI compatibility - Include comprehensive examples and tests - Update workspace dependencies to use unified versions - Add opc_classic_utils to opc_da dependencies This provides safe, efficient memory management for COM/OPC scenarios following standard COM conventions while maintaining zero runtime overhead.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.toml(1 hunks)opc_classic_utils/Cargo.toml(1 hunks)opc_classic_utils/README.md(1 hunks)opc_classic_utils/examples/basic_usage.rs(1 hunks)opc_classic_utils/examples/opc_scenarios.rs(1 hunks)opc_classic_utils/examples/transparent_repr_demo.rs(1 hunks)opc_classic_utils/src/lib.rs(1 hunks)opc_classic_utils/src/memory.rs(1 hunks)opc_da/Cargo.toml(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- opc_classic_utils/src/lib.rs
- opc_classic_utils/Cargo.toml
- opc_classic_utils/examples/basic_usage.rs
- opc_classic_utils/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- Cargo.toml
- opc_da/Cargo.toml
🧰 Additional context used
🧠 Learnings (2)
opc_classic_utils/examples/opc_scenarios.rs (2)
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/tests.rs:8-35
Timestamp: 2024-12-11T01:49:14.219Z
Learning: In the file `opc_da/src/client/tests.rs`, do not add retry logic in test functions as per the user's preference.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
opc_classic_utils/src/memory.rs (5)
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the `Drop` trait via `IUnknown`, automatically releasing resources when they go out of scope, so manual implementation of `Drop` for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling `std::mem::forget` on a value that implements `Copy`, such as `PWSTR`, has no effect because the value is copied and the original is still dropped. Therefore, using `std::mem::forget` in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In `windows-rs`, COM interfaces implement `IUnknown`, which automatically calls `Release` when dropped. Therefore, it's unnecessary to manually implement the `Drop` trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like `copy_to_pointer` are unnecessary and should be omitted.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
🧬 Code Graph Analysis (1)
opc_classic_utils/examples/transparent_repr_demo.rs (1)
opc_classic_utils/src/memory.rs (8)
from_raw(37-39)from_raw(108-110)from_raw(181-183)from_raw(258-260)as_ptr(42-44)as_ptr(113-115)as_ptr(193-195)as_ptr(270-272)
🔇 Additional comments (20)
opc_classic_utils/examples/transparent_repr_demo.rs (3)
10-22: LGTM! Well-designed mock C functions.The simulated C functions effectively demonstrate FFI compatibility scenarios. The
#[allow(dead_code)]attributes are appropriate since these are demonstration functions.
24-130: Excellent comprehensive demonstration of transparent representation.The demonstration effectively covers all key aspects:
- Memory layout compatibility verification
- FFI compatibility demonstration
- Zero-cost abstraction validation
- Wide string compatibility testing
- Clear benefit summary
The assertions and size comparisons provide concrete proof of the transparent representation working correctly.
132-137: Clean and effective main function.The main function properly orchestrates the demonstration and provides clear success messaging.
opc_classic_utils/examples/opc_scenarios.rs (4)
10-32: Excellent OPC server method simulation.The function effectively demonstrates the typical OPC pattern where:
- Input parameters are caller-allocated (client allocates, server uses)
- Output parameters are callee-allocated (server allocates, client frees)
The logging and parameter checking add educational value.
34-67: Clear client-server interaction demonstration.The simulation effectively shows the complete lifecycle:
- Client prepares input parameters
- Server processes and returns outputs
- Client receives and will automatically clean up outputs
The explanatory comments about automatic cleanup are particularly valuable.
69-91: Excellent educational content explaining parameter differences.The clear explanation of input vs output parameter semantics, ownership responsibilities, and key benefits provides valuable context for understanding the memory management patterns.
93-99: Well-structured main function.The main function properly coordinates the demonstrations and provides clear completion messaging.
opc_classic_utils/src/memory.rs (13)
14-23: Well-designed CallerAllocatedPtr structure.The transparent representation and clear documentation establish the correct semantics for caller-allocated memory where the callee is responsible for freeing.
25-59: Comprehensive and safe CallerAllocatedPtr implementation.The API provides all necessary methods:
- Safe constructors (
from_raw) and unsafe constructor (new) with clear safety documentation- Proper ownership transfer with
into_raw- Non-owning access with
as_ptr- Null checking functionality
The method signatures and documentation are clear and appropriate.
61-67: Correct Drop implementation for caller-allocated memory.The Drop implementation correctly does NOT free memory (since the callee is responsible) and nullifies the pointer to prevent use-after-free bugs. This matches the expected semantics.
83-92: Well-designed CalleeAllocatedPtr structure.The structure correctly represents memory allocated by the callee with appropriate documentation and transparent representation.
94-130: Comprehensive CalleeAllocatedPtr implementation.The implementation provides all necessary methods with appropriate safety documentation. The API is consistent with CallerAllocatedPtr while maintaining the correct ownership semantics.
132-141: Correct Drop implementation for callee-allocated memory.The Drop implementation correctly frees memory using CoTaskMemFree when the pointer is not null, matching the expected semantics for callee-allocated memory.
159-167: Well-designed CallerAllocatedWString structure.The wide string variant correctly mirrors the generic pointer wrapper with appropriate documentation for string-specific usage.
169-213: Comprehensive CallerAllocatedWString implementation.The implementation provides all necessary methods including Windows-specific PCWSTR conversions. The API is well-designed and consistent.
215-220: Correct Drop implementation for caller-allocated wide strings.The Drop implementation correctly does not free memory, matching the caller-allocated semantics.
236-244: Well-designed CalleeAllocatedWString structure.The structure correctly represents callee-allocated wide strings with appropriate documentation.
246-290: Comprehensive CalleeAllocatedWString implementation.The implementation provides all necessary methods with consistent API design and appropriate Windows integration.
292-301: Correct Drop implementation for callee-allocated wide strings.The Drop implementation correctly frees memory using CoTaskMemFree, matching the expected semantics.
317-382: Comprehensive test coverage.The test suite covers all the essential functionality:
- Default null pointer behavior
- Memory management semantics (no free for caller-allocated, free for callee-allocated)
- Transparent representation verification
The tests effectively validate the core functionality of the memory management wrappers.
- Replace from_rust_str method with standard FromStr trait implementation - Remove duplicate from_str method to avoid confusion with trait method - Update all call sites to use FromStr trait properly - Add comprehensive tests for FromStr trait functionality - Fix clippy warnings about method name conflicts - Improve API design to follow Rust conventions This change makes the API more idiomatic and eliminates clippy warnings about method name conflicts with std::str::FromStr::from_str.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
opc_classic_utils/src/memory.rs (4)
131-135: Clone implementation needs safety documentation.The Clone implementation creates a shallow copy of the pointer, which could lead to double-free issues if both instances are passed to functions that expect to free the memory. This matches the concerns raised in past review comments.
Consider adding documentation to the Clone implementation warning about potential double-free issues:
impl<T> Clone for CallerAllocatedPtr<T> { + /// Creates a shallow copy of the pointer. + /// + /// # Safety + /// + /// The caller must ensure that only one instance is passed to functions + /// that will free the memory, to avoid double-free errors. fn clone(&self) -> Self { Self { ptr: self.ptr } } }
230-236: Clone implementation needs safety documentation.Similar to CallerAllocatedPtr, the Clone implementation could lead to double-free issues since both instances will attempt to free the same memory when dropped.
Add safety documentation to the Clone implementation:
impl<T> Clone for CalleeAllocatedPtr<T> { + /// Creates a shallow copy of the pointer. + /// + /// # Safety + /// + /// This creates two instances that will both attempt to free the same memory + /// when dropped, potentially causing double-free errors. Use with extreme caution. fn clone(&self) -> Self { // Note: This creates a new wrapper around the same pointer. // The caller should ensure proper ownership semantics. Self { ptr: self.ptr } } }
378-382: Clone implementation needs safety documentation.Similar to the pointer types, the Clone implementation could lead to issues if both instances are passed to functions expecting to free the memory.
Add safety documentation consistent with the other Clone implementations:
impl Clone for CallerAllocatedWString { + /// Creates a shallow copy of the pointer. + /// + /// # Safety + /// + /// The caller must ensure that only one instance is passed to functions + /// that will free the memory, to avoid double-free errors. fn clone(&self) -> Self { Self { ptr: self.ptr } } }
516-520: Clone implementation needs safety documentation.Similar to CalleeAllocatedPtr, this Clone implementation could lead to double-free issues.
Add safety documentation:
impl Clone for CalleeAllocatedWString { + /// Creates a shallow copy of the pointer. + /// + /// # Safety + /// + /// This creates two instances that will both attempt to free the same memory + /// when dropped, potentially causing double-free errors. Use with extreme caution. fn clone(&self) -> Self { Self { ptr: self.ptr } } }
🧹 Nitpick comments (3)
opc_classic_utils/examples/convenience_functions.rs (2)
85-85: Unused import.The
FromStrimport is not used in this scope and should be removed.- use std::str::FromStr;
131-131: Duplicate import.The
FromStrimport is already imported at the top of the file and doesn't need to be imported again in this function scope.- use std::str::FromStr;opc_classic_utils/src/memory.rs (1)
284-287: Unnecessary import in function scope.The
FromStrimport is only used for the trait implementation later and doesn't need to be imported within this function scope.pub fn from_string(s: String) -> Result<Self, windows::core::Error> { - use std::str::FromStr; Self::from_str(&s) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
opc_classic_utils/examples/convenience_functions.rs(1 hunks)opc_classic_utils/src/memory.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
opc_classic_utils/examples/convenience_functions.rs (4)
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like `copy_to_pointer` are unnecessary and should be omitted.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/tests.rs:8-35
Timestamp: 2024-12-11T01:49:14.219Z
Learning: In the file `opc_da/src/client/tests.rs`, do not add retry logic in test functions as per the user's preference.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling `std::mem::forget` on a value that implements `Copy`, such as `PWSTR`, has no effect because the value is copied and the original is still dropped. Therefore, using `std::mem::forget` in such cases does not prevent the value from being dropped or prevent a potential double-free.
opc_classic_utils/src/memory.rs (4)
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the `Drop` trait via `IUnknown`, automatically releasing resources when they go out of scope, so manual implementation of `Drop` for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling `std::mem::forget` on a value that implements `Copy`, such as `PWSTR`, has no effect because the value is copied and the original is still dropped. Therefore, using `std::mem::forget` in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like `copy_to_pointer` are unnecessary and should be omitted.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In `windows-rs`, COM interfaces implement `IUnknown`, which automatically calls `Release` when dropped. Therefore, it's unnecessary to manually implement the `Drop` trait to release COM interfaces in structs that hold COM interfaces.
🧬 Code Graph Analysis (1)
opc_classic_utils/examples/convenience_functions.rs (1)
opc_classic_utils/src/memory.rs (24)
from_value(58-67)std(48-48)std(275-275)std(555-555)std(563-563)std(570-570)std(581-581)as_ptr(70-72)as_ptr(166-168)as_ptr(341-343)as_ptr(475-477)allocate(47-53)allocate(274-281)from_str(387-399)from_string(284-287)new(34-36)new(154-156)new(255-257)new(419-421)from_os_str(290-299)default(124-128)default(223-227)default(371-375)default(509-513)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: analyzing
🔇 Additional comments (3)
opc_classic_utils/examples/convenience_functions.rs (1)
1-187: Example demonstrates COM memory management utilities effectively.This example file provides a comprehensive demonstration of the
opc_classic_utilsmemory management utilities. The code is well-structured with clear explanations and proper assertions to verify correctness.Key strengths:
- Clear separation of concerns with dedicated functions for different aspects
- Proper use of unsafe blocks only where necessary
- Good documentation within the code via comments and print statements
- Realistic OPC scenario simulation
The unsafe code usage is appropriate and well-contained, following the established patterns from the memory module.
opc_classic_utils/src/memory.rs (2)
1-713: Comprehensive COM memory management utilities with proper patterns.This module provides excellent abstractions for COM memory management with clear separation between caller-allocated and callee-allocated patterns. The implementation correctly handles the ownership semantics and provides a safe, ergonomic API.
Key strengths:
- Proper memory management patterns for COM interop
- Clear documentation of ownership semantics
- Comprehensive test coverage
- Zero-cost abstractions with
#[repr(transparent)]- Consistent API across all pointer types
The implementation follows the established patterns from the retrieved learnings about COM memory management.
522-712: Comprehensive test coverage with good patterns.The test suite covers all major functionality including:
- Null pointer handling
- Memory management behavior verification
- Transparent representation
- String conversions
- Pointer dereferencing
- Edge cases with null pointers
The tests properly verify that caller-allocated types don't free memory while callee-allocated types do, which is crucial for the COM memory management patterns.
…memory management - Fix all unsafe_op_in_unsafe_fn warnings by adding unsafe blocks in unsafe functions - Improve CalleeAllocatedPtrArray Drop implementation to free each element before freeing the array - Remove unused imports in examples - Fix unused variable warnings in tests - All tests pass and examples run successfully
- Replace vec! macro with direct array literal for item_ids - All clippy warnings now resolved with strict settings - Tests and examples continue to pass
- Changed all Chinese output messages to English for better internationalization - Maintained the same functionality and educational value - All tests pass and clippy warnings resolved - Example now follows standard English coding conventions
- Split large memory.rs file (1604 lines) into logical modules: - memory/mod.rs: Main module with re-exports - memory/ptr.rs: Pointer types (CallerAllocatedPtr, CalleeAllocatedPtr) - memory/wstring.rs: Wide string types (CallerAllocatedWString, CalleeAllocatedWString) - memory/array.rs: Array types (CallerAllocatedArray, CalleeAllocatedArray) - memory/ptr_array.rs: Pointer array types (CallerAllocatedPtrArray, CalleeAllocatedPtrArray) - memory/tests.rs: All test functions - Maintained all existing functionality and API - All tests pass (29 tests) - All examples work correctly - No clippy warnings - Improved code organization and maintainability - Each module focuses on a specific type category - Easier to navigate and understand the codebase
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (12)
opc_classic_utils/examples/array_functions.rs (3)
9-9: Remove unused importThe
FromStrimport is not used at the file level and is redundantly imported within a function scope.-use std::str::FromStr;
84-88: Simplify mutable array access patternThe current pattern of getting an item, modifying it, then setting it back can be simplified by directly accessing the mutable reference.
unsafe { - let mut item = *caller_array.get(1).unwrap(); - item.value = 25.0; - *caller_array.as_mut_slice().unwrap().get_mut(1).unwrap() = item; + caller_array.get_mut(1).unwrap().value = 25.0; assert_eq!(caller_array.get(1).unwrap().value, 25.0); }
125-132: Simplify null pointer usageThe named null pointer variables are unnecessary since you can pass
std::ptr::null_mut()directly to thesetmethod.unsafe { - let test_ptr1 = std::ptr::null_mut::<i32>(); - let test_ptr2 = std::ptr::null_mut::<i32>(); - let test_ptr3 = std::ptr::null_mut::<i32>(); - - assert!(uninit_ptr_array.set(0, test_ptr1)); - assert!(uninit_ptr_array.set(1, test_ptr2)); - assert!(uninit_ptr_array.set(2, test_ptr3)); - assert!(!uninit_ptr_array.set(3, test_ptr1)); // Out of bounds + assert!(uninit_ptr_array.set(0, std::ptr::null_mut())); + assert!(uninit_ptr_array.set(1, std::ptr::null_mut())); + assert!(uninit_ptr_array.set(2, std::ptr::null_mut())); + assert!(!uninit_ptr_array.set(3, std::ptr::null_mut())); // Out of bounds }opc_classic_utils/examples/memory_management_comparison.rs (1)
112-128: Consider adding explicit verification to the testThe current test relies on implicit behavior without explicit assertions. Consider adding verification that the types behave as expected.
#[test] fn test_memory_management_differences() { // This test demonstrates the different memory management behaviors // Test CalleeAllocatedArray - should only free the container { let _array = CalleeAllocatedArray::<i32>::default(); + assert!(_array.is_null()); + assert_eq!(_array.len(), 0); // When _array goes out of scope, only the container is freed } // Test CalleeAllocatedPtrArray - should free both container and elements { let _ptr_array = CalleeAllocatedPtrArray::<i32>::default(); + assert!(_ptr_array.is_null()); + assert_eq!(_ptr_array.len(), 0); // When _ptr_array goes out of scope, both container and elements are freed } // Both should complete without memory leaks + // The actual memory management is verified by the Drop implementations }opc_classic_utils/src/memory/tests.rs (1)
89-91: Remove redundant import
std::str::FromStris already imported at the top of the file.// Test creating wide string from Rust string -use std::str::FromStr; let test_string = "Hello, World!";opc_classic_utils/src/memory/ptr.rs (4)
81-87: Remove redundant unsafe blockThe method is already marked
unsafe, so the internal unsafe block is redundant.pub unsafe fn as_ref(&self) -> Option<&T> { if self.ptr.is_null() { None } else { - Some(unsafe { &*self.ptr }) + Some(&*self.ptr) } }
94-100: Remove redundant unsafe blockThe method is already marked
unsafe, so the internal unsafe block is redundant.pub unsafe fn as_mut(&mut self) -> Option<&mut T> { if self.ptr.is_null() { None } else { - Some(unsafe { &mut *self.ptr }) + Some(&mut *self.ptr) } }
177-183: Remove redundant unsafe blockThe method is already marked
unsafe, so the internal unsafe block is redundant.pub unsafe fn as_ref(&self) -> Option<&T> { if self.ptr.is_null() { None } else { - Some(unsafe { &*self.ptr }) + Some(&*self.ptr) } }
190-196: Remove redundant unsafe blockThe method is already marked
unsafe, so the internal unsafe block is redundant.pub unsafe fn as_mut(&mut self) -> Option<&mut T> { if self.ptr.is_null() { None } else { - Some(unsafe { &mut *self.ptr }) + Some(&mut *self.ptr) } }opc_classic_utils/src/memory/wstring.rs (3)
54-57: Simplify string conversionThe explicit trait import and call can be simplified.
/// Creates a `CallerAllocatedWString` from a Rust string pub fn from_string(s: String) -> Result<Self, windows::core::Error> { - use std::str::FromStr; - Self::from_str(&s) + Self::from_str(s.as_str()) }
76-89: Extract common string length calculation and remove redundant unsafe blocksBoth
to_stringandto_os_stringcontain duplicated logic for calculating the string length and have redundant unsafe blocks.+ /// Calculate the length of a null-terminated wide string + /// + /// # Safety + /// The caller must ensure the pointer is valid and points to a null-terminated wide string. + unsafe fn wide_string_len(ptr: *const u16) -> usize { + let mut len = 0; + while *ptr.add(len) != 0 { + len += 1; + } + len + } + /// Converts the wide string to a Rust string slice /// /// # Safety /// /// The caller must ensure the pointer is valid and points to a null-terminated wide string. pub unsafe fn to_string(&self) -> Option<String> { if self.ptr.is_null() { return None; } - let mut len = 0; - while unsafe { *self.ptr.add(len) } != 0 { - len += 1; - } + let len = Self::wide_string_len(self.ptr); - let slice = unsafe { std::slice::from_raw_parts(self.ptr, len) }; + let slice = std::slice::from_raw_parts(self.ptr, len); let os_string = OsString::from_wide(slice); Some(os_string.to_string_lossy().into_owned()) } /// Converts the wide string to an `OsString` /// /// # Safety /// /// The caller must ensure the pointer is valid and points to a null-terminated wide string. pub unsafe fn to_os_string(&self) -> Option<OsString> { if self.ptr.is_null() { return None; } - let mut len = 0; - while unsafe { *self.ptr.add(len) } != 0 { - len += 1; - } + let len = Self::wide_string_len(self.ptr); - let slice = unsafe { std::slice::from_raw_parts(self.ptr, len) }; + let slice = std::slice::from_raw_parts(self.ptr, len); Some(OsString::from_wide(slice)) }Also applies to: 96-108
210-223: Extract common string length calculation and remove redundant unsafe blocksSimilar to
CallerAllocatedWString, this type has duplicated logic that should be extracted.+ /// Calculate the length of a null-terminated wide string + /// + /// # Safety + /// The caller must ensure the pointer is valid and points to a null-terminated wide string. + unsafe fn wide_string_len(ptr: *const u16) -> usize { + let mut len = 0; + while *ptr.add(len) != 0 { + len += 1; + } + len + } + /// Converts the wide string to a Rust string slice /// /// # Safety /// /// The caller must ensure the pointer is valid and points to a null-terminated wide string. pub unsafe fn to_string(&self) -> Option<String> { if self.ptr.is_null() { return None; } - let mut len = 0; - while unsafe { *self.ptr.add(len) } != 0 { - len += 1; - } + let len = Self::wide_string_len(self.ptr); - let slice = unsafe { std::slice::from_raw_parts(self.ptr, len) }; + let slice = std::slice::from_raw_parts(self.ptr, len); let os_string = OsString::from_wide(slice); Some(os_string.to_string_lossy().into_owned()) } /// Converts the wide string to an `OsString` /// /// # Safety /// /// The caller must ensure the pointer is valid and points to a null-terminated wide string. pub unsafe fn to_os_string(&self) -> Option<OsString> { if self.ptr.is_null() { return None; } - let mut len = 0; - while unsafe { *self.ptr.add(len) } != 0 { - len += 1; - } + let len = Self::wide_string_len(self.ptr); - let slice = unsafe { std::slice::from_raw_parts(self.ptr, len) }; + let slice = std::slice::from_raw_parts(self.ptr, len); Some(OsString::from_wide(slice)) }Also applies to: 230-242
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
opc_classic_utils/examples/array_functions.rs(1 hunks)opc_classic_utils/examples/memory_management_comparison.rs(1 hunks)opc_classic_utils/src/memory/array.rs(1 hunks)opc_classic_utils/src/memory/mod.rs(1 hunks)opc_classic_utils/src/memory/ptr.rs(1 hunks)opc_classic_utils/src/memory/ptr_array.rs(1 hunks)opc_classic_utils/src/memory/tests.rs(1 hunks)opc_classic_utils/src/memory/wstring.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- opc_classic_utils/src/memory/mod.rs
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the `Drop` trait via `IUnknown`, automatically releasing resources when they go out of scope, so manual implementation of `Drop` for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like `copy_to_pointer` are unnecessary and should be omitted.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the `opc_da/src/com/server.rs` file (Rust), the enum variant `OPC_NS_HIERARCHIAL` is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: Ronbb/rust_opc#10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In `opc_da/src/client/iterator.rs`, when initializing the `cache` array in `GroupIterator`, use `cache: [const { None }; 16]` instead of `cache: [None; 16]` because Flipper requires this change.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In `windows-rs`, COM interfaces implement `IUnknown`, which automatically calls `Release` when dropped. Therefore, it's unnecessary to manually implement the `Drop` trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling `std::mem::forget` on a value that implements `Copy`, such as `PWSTR`, has no effect because the value is copied and the original is still dropped. Therefore, using `std::mem::forget` in such cases does not prevent the value from being dropped or prevent a potential double-free.
opc_classic_utils/examples/memory_management_comparison.rs (3)
Learnt from: Ronbb
PR: Ronbb/rust_opc#10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In `opc_da/src/client/iterator.rs`, when initializing the `cache` array in `GroupIterator`, use `cache: [const { None }; 16]` instead of `cache: [None; 16]` because Flipper requires this change.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling `std::mem::forget` on a value that implements `Copy`, such as `PWSTR`, has no effect because the value is copied and the original is still dropped. Therefore, using `std::mem::forget` in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
opc_classic_utils/examples/array_functions.rs (5)
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/unified/group.rs:28-39
Timestamp: 2024-12-10T13:51:14.061Z
Learning: In the `add_items` method in `opc_da/src/client/unified/group.rs`, the OPC DA protocol guarantees that the `results` and `errors` arrays returned will always be of the same length, so additional validation for array length mismatch is not necessary.
Learnt from: Ronbb
PR: Ronbb/rust_opc#10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In `opc_da/src/client/iterator.rs`, when initializing the `cache` array in `GroupIterator`, use `cache: [const { None }; 16]` instead of `cache: [None; 16]` because Flipper requires this change.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/tests.rs:8-35
Timestamp: 2024-12-11T01:49:14.219Z
Learning: In the file `opc_da/src/client/tests.rs`, do not add retry logic in test functions as per the user's preference.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like `copy_to_pointer` are unnecessary and should be omitted.
opc_classic_utils/src/memory/tests.rs (3)
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/tests.rs:8-35
Timestamp: 2024-12-11T01:49:14.219Z
Learning: In the file `opc_da/src/client/tests.rs`, do not add retry logic in test functions as per the user's preference.
Learnt from: Ronbb
PR: Ronbb/rust_opc#10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In `opc_da/src/client/iterator.rs`, when initializing the `cache` array in `GroupIterator`, use `cache: [const { None }; 16]` instead of `cache: [None; 16]` because Flipper requires this change.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling `std::mem::forget` on a value that implements `Copy`, such as `PWSTR`, has no effect because the value is copied and the original is still dropped. Therefore, using `std::mem::forget` in such cases does not prevent the value from being dropped or prevent a potential double-free.
opc_classic_utils/src/memory/ptr.rs (4)
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the `Drop` trait via `IUnknown`, automatically releasing resources when they go out of scope, so manual implementation of `Drop` for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In `windows-rs`, COM interfaces implement `IUnknown`, which automatically calls `Release` when dropped. Therefore, it's unnecessary to manually implement the `Drop` trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling `std::mem::forget` on a value that implements `Copy`, such as `PWSTR`, has no effect because the value is copied and the original is still dropped. Therefore, using `std::mem::forget` in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like `copy_to_pointer` are unnecessary and should be omitted.
opc_classic_utils/src/memory/wstring.rs (4)
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the `Drop` trait via `IUnknown`, automatically releasing resources when they go out of scope, so manual implementation of `Drop` for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In `windows-rs`, COM interfaces implement `IUnknown`, which automatically calls `Release` when dropped. Therefore, it's unnecessary to manually implement the `Drop` trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling `std::mem::forget` on a value that implements `Copy`, such as `PWSTR`, has no effect because the value is copied and the original is still dropped. Therefore, using `std::mem::forget` in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like `copy_to_pointer` are unnecessary and should be omitted.
opc_classic_utils/src/memory/array.rs (4)
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the `Drop` trait via `IUnknown`, automatically releasing resources when they go out of scope, so manual implementation of `Drop` for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like `copy_to_pointer` are unnecessary and should be omitted.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In `windows-rs`, COM interfaces implement `IUnknown`, which automatically calls `Release` when dropped. Therefore, it's unnecessary to manually implement the `Drop` trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling `std::mem::forget` on a value that implements `Copy`, such as `PWSTR`, has no effect because the value is copied and the original is still dropped. Therefore, using `std::mem::forget` in such cases does not prevent the value from being dropped or prevent a potential double-free.
opc_classic_utils/src/memory/ptr_array.rs (4)
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the `Drop` trait via `IUnknown`, automatically releasing resources when they go out of scope, so manual implementation of `Drop` for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In `windows-rs`, COM interfaces implement `IUnknown`, which automatically calls `Release` when dropped. Therefore, it's unnecessary to manually implement the `Drop` trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like `copy_to_pointer` are unnecessary and should be omitted.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling `std::mem::forget` on a value that implements `Copy`, such as `PWSTR`, has no effect because the value is copied and the original is still dropped. Therefore, using `std::mem::forget` in such cases does not prevent the value from being dropped or prevent a potential double-free.
🧬 Code Graph Analysis (4)
opc_classic_utils/examples/array_functions.rs (4)
opc_classic_utils/src/memory/array.rs (1)
from_slice(60-76)opc_classic_utils/src/memory/ptr.rs (1)
from_value(46-55)opc_classic_utils/src/memory/ptr_array.rs (1)
from_ptr_slice(62-75)opc_classic_utils/src/memory/wstring.rs (1)
from_str(157-169)
opc_classic_utils/src/memory/ptr.rs (4)
opc_classic_utils/src/memory/ptr_array.rs (12)
std(43-43)new(22-24)new(235-237)allocate(35-57)is_null(104-106)is_null(273-275)drop(165-170)drop(334-350)default(174-179)default(354-359)clone(183-188)clone(363-368)opc_classic_utils/src/memory/array.rs (12)
std(43-43)new(22-24)new(227-229)allocate(35-55)is_null(105-107)is_null(265-267)drop(163-168)drop(323-331)default(172-177)default(335-340)clone(181-186)clone(344-349)opc_classic_utils/src/memory/wstring.rs (12)
std(45-45)new(25-27)new(189-191)allocate(44-51)is_null(123-125)is_null(257-259)drop(134-137)drop(268-275)default(141-145)default(279-283)clone(149-151)clone(287-289)opc_classic_utils/src/memory/tests.rs (7)
std(32-32)std(40-40)std(47-47)std(58-58)std(319-319)std(341-341)std(355-355)
opc_classic_utils/src/memory/wstring.rs (5)
opc_classic_utils/src/memory/ptr.rs (18)
std(36-36)new(22-24)new(142-144)from_raw(27-29)from_raw(149-151)allocate(35-41)as_ptr(58-60)as_ptr(154-156)into_raw(65-69)into_raw(161-165)is_null(72-74)is_null(168-170)drop(104-108)drop(200-207)default(112-116)default(211-215)clone(120-122)clone(219-221)opc_classic_utils/src/memory/ptr_array.rs (18)
std(43-43)new(22-24)new(235-237)from_raw(27-29)from_raw(242-244)allocate(35-57)as_ptr(78-80)as_ptr(247-249)into_raw(85-91)into_raw(254-260)is_null(104-106)is_null(273-275)drop(165-170)drop(334-350)default(174-179)default(354-359)clone(183-188)clone(363-368)opc_classic_utils/src/memory/array.rs (18)
std(43-43)new(22-24)new(227-229)from_raw(27-29)from_raw(234-236)allocate(35-55)as_ptr(79-81)as_ptr(239-241)into_raw(86-92)into_raw(246-252)is_null(105-107)is_null(265-267)drop(163-168)drop(323-331)default(172-177)default(335-340)clone(181-186)clone(344-349)opc_classic_utils/src/memory/tests.rs (7)
std(32-32)std(40-40)std(47-47)std(58-58)std(319-319)std(341-341)std(355-355)opc_da/src/server/com/utils.rs (1)
s(143-146)
opc_classic_utils/src/memory/ptr_array.rs (3)
opc_classic_utils/src/memory/ptr.rs (18)
std(36-36)new(22-24)new(142-144)from_raw(27-29)from_raw(149-151)allocate(35-41)as_ptr(58-60)as_ptr(154-156)into_raw(65-69)into_raw(161-165)is_null(72-74)is_null(168-170)drop(104-108)drop(200-207)default(112-116)default(211-215)clone(120-122)clone(219-221)opc_classic_utils/src/memory/array.rs (28)
std(43-43)new(22-24)new(227-229)len(95-97)len(255-257)from_raw(27-29)from_raw(234-236)allocate(35-55)as_ptr(79-81)as_ptr(239-241)into_raw(86-92)into_raw(246-252)is_empty(100-102)is_empty(260-262)is_null(105-107)is_null(265-267)as_slice(114-120)as_slice(274-280)as_mut_slice(127-133)as_mut_slice(287-293)get(140-146)get(300-306)drop(163-168)drop(323-331)default(172-177)default(335-340)clone(181-186)clone(344-349)opc_classic_utils/src/memory/wstring.rs (18)
std(45-45)new(25-27)new(189-191)from_raw(30-32)from_raw(194-196)allocate(44-51)as_ptr(111-113)as_ptr(245-247)into_raw(116-120)into_raw(250-254)is_null(123-125)is_null(257-259)drop(134-137)drop(268-275)default(141-145)default(279-283)clone(149-151)clone(287-289)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: analyzing
🔇 Additional comments (1)
opc_classic_utils/src/memory/ptr_array.rs (1)
333-351: Well-implemented memory cleanup logicThe
Dropimplementation correctly handles the two-level memory management:
- Frees each individual pointer in the array (checking for null)
- Frees the array container itself
This pattern is appropriate for COM functions that return arrays of allocated pointers (e.g., arrays of strings).
- Remove dangerous Clone implementations for CalleeAllocated types to prevent double-free errors - Add safety documentation for CallerAllocated Clone implementations - Fix dependency version inconsistencies in Cargo.toml - Remove redundant unsafe blocks in memory management code - Improve code safety and prevent memory corruption issues All tests pass for opc_classic_utils crate. Memory management is now safer and follows Rust best practices for COM interop.
- Fix misleading test function names and comments for memory management tests - Improve lifetime management in array_functions example to prevent dangling pointers - Ensure all GitHub PR review suggestions are addressed All tests pass, no clippy warnings, examples run successfully.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
opc_classic_utils/examples/array_functions.rs (1)
156-160: Consider storing wrapper objects to ensure proper lifetime managementWhile the current code works because
CallerAllocatedWStringdoesn't free memory on drop, extracting raw pointers without keeping the wrapper objects alive could be confusing and potentially unsafe if the implementation changes.// Client prepares item IDs for batch read let item_ids = ["Item1", "Item2", "Item3", "Item4", "Item5"]; -let item_id_wstrings: Vec<CallerAllocatedWString> = item_ids +let item_id_wstrings: Vec<CallerAllocatedWString> = item_ids .iter() - .map(|id| CallerAllocatedWString::from_str(id).unwrap()) + .map(|id| CallerAllocatedWString::from_str(id).unwrap()) .collect(); -let item_id_ptrs: Vec<*mut u16> = item_id_wstrings.iter().map(|ws| ws.as_ptr()).collect(); +let item_id_ptrs: Vec<*mut u16> = item_id_wstrings.iter().map(|ws| ws.as_ptr()).collect();Wait, the suggested diff is already implemented in the current code. Let me re-read...
Actually, the current code does store the wrapper objects in
item_id_wstringsbefore extracting pointers, which is the correct approach. The past review comment appears to be outdated.
🧹 Nitpick comments (1)
opc_classic_utils/src/memory/tests.rs (1)
87-101: Remove redundant importThe
FromStrtrait is already imported at line 2. Remove the duplicate import at line 90.#[test] fn test_caller_allocated_wstring_from_str() { // Test creating wide string from Rust string - use std::str::FromStr; let test_string = "Hello, World!"; let wstring = CallerAllocatedWString::from_str(test_string).unwrap();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
opc_classic_utils/examples/array_functions.rs(1 hunks)opc_classic_utils/src/memory/tests.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the `Drop` trait via `IUnknown`, automatically releasing resources when they go out of scope, so manual implementation of `Drop` for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like `copy_to_pointer` are unnecessary and should be omitted.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the `opc_da/src/com/server.rs` file (Rust), the enum variant `OPC_NS_HIERARCHIAL` is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: Ronbb/rust_opc#10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In `opc_da/src/client/iterator.rs`, when initializing the `cache` array in `GroupIterator`, use `cache: [const { None }; 16]` instead of `cache: [None; 16]` because Flipper requires this change.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In `windows-rs`, COM interfaces implement `IUnknown`, which automatically calls `Release` when dropped. Therefore, it's unnecessary to manually implement the `Drop` trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling `std::mem::forget` on a value that implements `Copy`, such as `PWSTR`, has no effect because the value is copied and the original is still dropped. Therefore, using `std::mem::forget` in such cases does not prevent the value from being dropped or prevent a potential double-free.
opc_classic_utils/examples/array_functions.rs (7)
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/unified/group.rs:28-39
Timestamp: 2024-12-10T13:51:14.061Z
Learning: In the `add_items` method in `opc_da/src/client/unified/group.rs`, the OPC DA protocol guarantees that the `results` and `errors` arrays returned will always be of the same length, so additional validation for array length mismatch is not necessary.
Learnt from: Ronbb
PR: Ronbb/rust_opc#10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In `opc_da/src/client/iterator.rs`, when initializing the `cache` array in `GroupIterator`, use `cache: [const { None }; 16]` instead of `cache: [None; 16]` because Flipper requires this change.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/tests.rs:8-35
Timestamp: 2024-12-11T01:49:14.219Z
Learning: In the file `opc_da/src/client/tests.rs`, do not add retry logic in test functions as per the user's preference.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/traits/item_mgt.rs:191-194
Timestamp: 2024-12-10T03:00:26.689Z
Learning: In `opc_da/src/client/traits/item_mgt.rs`, the `CreateEnumerator` method returns a `Result`, so there is no need to handle `None` values.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling `std::mem::forget` on a value that implements `Copy`, such as `PWSTR`, has no effect because the value is copied and the original is still dropped. Therefore, using `std::mem::forget` in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like `copy_to_pointer` are unnecessary and should be omitted.
opc_classic_utils/src/memory/tests.rs (3)
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/tests.rs:8-35
Timestamp: 2024-12-11T01:49:14.219Z
Learning: In the file `opc_da/src/client/tests.rs`, do not add retry logic in test functions as per the user's preference.
Learnt from: Ronbb
PR: Ronbb/rust_opc#10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In `opc_da/src/client/iterator.rs`, when initializing the `cache` array in `GroupIterator`, use `cache: [const { None }; 16]` instead of `cache: [None; 16]` because Flipper requires this change.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling `std::mem::forget` on a value that implements `Copy`, such as `PWSTR`, has no effect because the value is copied and the original is still dropped. Therefore, using `std::mem::forget` in such cases does not prevent the value from being dropped or prevent a potential double-free.
🧬 Code Graph Analysis (2)
opc_classic_utils/examples/array_functions.rs (5)
opc_classic_utils/src/memory/tests.rs (7)
std(32-32)std(40-40)std(47-47)std(58-58)std(319-319)std(341-341)std(355-355)opc_classic_utils/src/memory/array.rs (1)
from_slice(60-76)opc_classic_utils/src/memory/ptr.rs (1)
from_value(46-55)opc_classic_utils/src/memory/ptr_array.rs (1)
from_ptr_slice(62-75)opc_classic_utils/src/memory/wstring.rs (1)
from_str(163-175)
opc_classic_utils/src/memory/tests.rs (4)
opc_classic_utils/src/memory/ptr.rs (15)
std(36-36)default(112-116)default(217-221)is_null(72-74)is_null(174-176)from_raw(27-29)from_raw(155-157)as_ptr(58-60)as_ptr(160-162)allocate(35-41)from_value(46-55)new(22-24)new(148-150)as_ref(81-87)as_ref(183-189)opc_classic_utils/src/memory/array.rs (21)
std(43-43)default(172-177)default(341-346)is_null(105-107)is_null(271-273)from_raw(27-29)from_raw(240-242)as_ptr(79-81)as_ptr(245-247)allocate(35-55)new(22-24)new(233-235)is_empty(100-102)is_empty(266-268)len(95-97)len(261-263)from_slice(60-76)as_slice(114-120)as_slice(280-286)get(140-146)get(306-312)opc_classic_utils/src/memory/ptr_array.rs (23)
std(43-43)default(174-179)default(360-365)is_null(104-106)is_null(279-281)from_raw(27-29)from_raw(248-250)as_ptr(78-80)as_ptr(253-255)allocate(35-57)new(22-24)new(241-243)is_empty(99-101)is_empty(274-276)len(94-96)len(269-271)as_slice(113-119)as_slice(288-294)get(139-145)get(314-320)from_ptr_slice(62-75)set(152-161)set(327-336)opc_classic_utils/src/memory/wstring.rs (17)
std(45-45)default(141-145)default(285-289)is_null(123-125)is_null(263-265)from_raw(30-32)from_raw(200-202)as_ptr(111-113)as_ptr(251-253)allocate(44-51)new(25-27)new(195-197)from_os_str(60-69)to_string(76-89)to_string(216-229)to_os_string(96-108)to_os_string(236-248)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: analyzing
🔇 Additional comments (16)
opc_classic_utils/src/memory/tests.rs (16)
1-27: LGTM!The imports and initial null state tests are well-structured and cover all wrapper types appropriately.
28-43: Tests correctly document ownership semanticsThese tests appropriately document the memory ownership model, though they use null pointers which won't exercise the actual CoTaskMemFree path. This is acceptable as documentation tests.
44-65: LGTM!Excellent coverage of transparent representation verification for all wrapper types.
66-86: LGTM!Allocation tests are well-structured with proper unsafe usage and value verification.
102-129: LGTM!String and OsStr conversion tests are comprehensive with proper round-trip verification.
150-161: LGTM!Proper verification of null pointer handling in dereference operations.
162-189: LGTM!Good coverage of null wide string conversions and FromStr trait implementation.
190-205: LGTM!Comprehensive null state verification for array types.
206-227: LGTM!Array allocation and slice conversion tests are well-implemented with proper data verification.
228-257: LGTM!Excellent comprehensive coverage of array access patterns including bounds checking and mutation operations.
258-265: LGTM!Test name and comment correctly reflect the memory management behavior of CalleeAllocatedArray.
266-281: LGTM!Proper null state verification for pointer array types.
282-307: LGTM!Pointer array allocation and slice conversion tests are correctly implemented.
308-328: LGTM!Good test coverage for pointer array access with accurate comment about uninitialized memory.
329-337: LGTM!Test name and comment accurately describe the comprehensive memory freeing behavior of CalleeAllocatedPtrArray.
338-364: LGTM!Comprehensive verification of transparent representation for all array wrapper types.
- Fix double-free vulnerability in test_pointer_dereference by removing shared ownership - Add separate test for CalleeAllocatedPtr dereferencing with proper memory allocation - Add from_value method to CalleeAllocatedPtr for safe memory allocation in tests - Ensure all memory safety issues identified in PR reviews are resolved All 30 tests pass for opc_classic_utils crate.
coderabbitai
bot
changed the title
1 participant
Summary by CodeRabbit
New Features
opc_classic_utilscrate providing utilities for OPC Classic memory management, including smart pointer and array types that handle caller-allocated and callee-allocated COM memory.Documentation
Tests
Chores