refactor: add safe OPC Classic client and server bindings - #24
Conversation
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe workspace moves to Rust 2024 and adds shared COM types, ABI support, ownership utilities, and safe client/server layers for OPC Common, AE, DA, and HDA. ChangesWorkspace and shared COM foundation
OPC Common
OPC AE
OPC DA
OPC HDA
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (14)
opc_ae_bindings/src/client/mod.rs (3)
74-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the shadowed owner bindings.
Lines 82-83 shadow
areasandsourceswithPCWSTRvectors. The originalWideCStringvectors stay alive until the end of the function, so the pointers are valid today. A later refactor that moves the pointer vectors out of this scope would create dangling pointers. Distinct names make the ownership explicit.♻️ Proposed rename
- let areas = areas+ let area_strings = areas .iter() .map(|area| wide(area)) .collect::<Result<Vec<_>>>()?; - let sources = sources+ let source_strings = sources .iter() .map(|source| wide(source)) .collect::<Result<Vec<_>>>()?; - let areas: Vec<_> = areas.iter().map(WideCString::as_pcwstr).collect();- let sources: Vec<_> = sources.iter().map(WideCString::as_pcwstr).collect();+ let areas: Vec<_> = area_strings.iter().map(WideCString::as_pcwstr).collect();+ let sources: Vec<_> = source_strings.iter().map(WideCString::as_pcwstr).collect();🤖 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 `@opc_ae_bindings/src/client/mod.rs` around lines 74 - 83, Rename the second `areas` and `sources` bindings in the `WideCString::as_pcwstr` conversion to distinct pointer-vector names, while keeping the original owned `WideCString` vectors unchanged and alive for the pointer usage that follows.
370-384: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
query_string_arrayhere.
condition_namesrepeats the body ofquery_string_arrayat lines 482-496. Call the helper instead, assubcondition_namesandsource_conditionsdo.♻️ Proposed refactor
pub fn condition_names(&self, event_category: u32) -> Result<Vec<String>> { - let mut count = 0u32;- let mut names = CoTaskMemOut::<windows_core::PWSTR>::new();- let call = unsafe {- self.inner- .QueryConditionNames(event_category, &mut count, names.as_mut_ptr())- };- let names = unsafe { names.into_array(count as usize, FreePwstrElements) }?;- call?;- Ok(names- .as_slice()- .iter()- .map(|name| pwstr_string(*name))- .collect())+ self.query_string_array(|count, output| unsafe {+ self.inner+ .QueryConditionNames(event_category, count, output)+ }) }🤖 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 `@opc_ae_bindings/src/client/mod.rs` around lines 370 - 384, Update condition_names to delegate to the existing query_string_array helper instead of duplicating the QueryConditionNames, CoTaskMemOut, and PWSTR conversion logic. Match the usage pattern in subcondition_names and source_conditions while preserving the current event_category argument and Result<Vec<String>> behavior.
522-528: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefer lossy decoding for
pwstr_string.
unwrap_or_defaultreturns an emptyStringwhenPWSTR::to_string()rejects invalid UTF-16, making invalid content indistinguishable from an empty value. Use lossy decoding instead to keep the readable content.🤖 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 `@opc_ae_bindings/src/client/mod.rs` around lines 522 - 528, Update pwstr_string to decode non-null PWSTR values lossily instead of calling to_string().unwrap_or_default(), preserving readable content when the UTF-16 data is invalid while retaining the empty result for null pointers.opc_ae_bindings/src/server/mod.rs (2)
283-300: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
StatusCleanupin the AE client and server modules. Both modules define an identicalCleanupimpl for__MIDL___MIDL_itf_opc_ae_0000_0001_0005; the shared root cause is a missing common definition for the AE status vendor-string cleanup.
opc_ae_bindings/src/server/mod.rs#L283-L300: move this impl into a shared module of the crate and import it here.opc_ae_bindings/src/client/mod.rs#L275-L292: delete this copy and import the shared impl.🤖 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 `@opc_ae_bindings/src/server/mod.rs` around lines 283 - 300, Create a shared crate module containing the StatusCleanup type and its Cleanup implementation for __MIDL___MIDL_itf_opc_ae_0000_0001_0005, preserving the existing vendor-string cleanup behavior. In opc_ae_bindings/src/server/mod.rs#L283-L300, remove the local definition and import the shared implementation; in opc_ae_bindings/src/client/mod.rs#L275-L292, delete the duplicate and import the same shared implementation.
399-415: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the decoded input names and cover the remaining methods.
TestService::enable_areaignores itsnamesargument, so line 414 proves only that the call returnsOk. It does not prove thatdecode_stringsproduced"plant". Record the received names in the test service and assert them. Also add cases forsource_conditions,disable_areas,enable_sources,disable_sources, and for the methods that returnE_NOTIMPL.🤖 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 `@opc_ae_bindings/src/server/mod.rs` around lines 399 - 415, Extend client_and_server_adapters_round_trip_task_memory and TestService so enable_area records its decoded names, then assert that enabling "plant" reaches the service unchanged. Add coverage for source_conditions, disable_areas, enable_sources, and disable_sources, and exercise the remaining adapter methods that are expected to return E_NOTIMPL, asserting those HRESULTs explicitly.opc_da_bindings/src/server/mod.rs (1)
880-901: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the round-trip test to cover the error and write paths.
The test covers status, group creation, add-items, read, and close, and all service methods return success. The partial-success path through
batch_status, theWritepath, and the blob cleanup inItemResultCleanupstay untested. Add a service variant that returnsServerItemResult::failurefor one item, then assert that the client maps it toErr(ItemError { .. })and that the successful entries still decode.🤖 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 `@opc_da_bindings/src/server/mod.rs` around lines 880 - 901, Extend client_and_server_adapters_round_trip_owned_values with a TestService variant that returns ServerItemResult::failure for one item while preserving a successful result for another; exercise batch_status and Write, assert the failed entry maps to Err(ItemError { .. }) while successful entries decode, and verify blob cleanup through ItemResultCleanup.opc_da_bindings/src/client/group.rs (1)
107-246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared item-definition and result-mapping code.
add_itemsandvalidate_itemsrepeat the same three blocks: theids/pathsconversion, thetagOPCITEMDEFconstruction, and the result-to-AddedItemmapping. Only the COM call differs. Extract two helpers, for examplebuild_definitions(specs) -> Result<(Vec<WideCString>, Vec<WideCString>, Vec<tagOPCITEMDEF>)>andmap_item_results(results, errors) -> Vec<...>. This removes about 60 duplicated lines and keeps the two paths from diverging.Also make the count argument consistent. Line 136 uses
definitions.len()and line 211 usesspecs.len()for the same value.🤖 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 `@opc_da_bindings/src/client/group.rs` around lines 107 - 246, Extract the duplicated ID/path conversion and tagOPCITEMDEF construction from add_items and validate_items into a shared build_definitions helper, and extract their identical result/error-to-AddedItem mapping into map_item_results. Update both methods to use these helpers while preserving their distinct COM calls, and use the shared definitions length consistently when passing the count argument.opc_hda_bindings/src/server/mod.rs (2)
126-132: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueFinish all builders before you write any output pointer.
finish()is called inline for each output. If a laterfinish()fails, the earlier arrays are already written into caller memory while the method returns an error HRESULT. The caller must ignore out-params on failure, so those allocations leak. The same pattern exists inGetAggregatesat Lines 162-167 and inGetHistorianStatusat Lines 203-213.♻️ Proposed ordering change
+ let id_values = id_values.finish()?;+ let name_values = name_values.finish()?;+ let description_values = description_values.finish()?;+ let type_values = type_values.finish()?; unsafe { count.write(count_value); - ids.write(id_values.finish()?.into_raw_parts().0);- names.write(name_values.finish()?.into_raw_parts().0);- descriptions.write(description_values.finish()?.into_raw_parts().0);- data_types.write(type_values.finish()?.into_raw_parts().0);+ ids.write(id_values.into_raw_parts().0);+ names.write(name_values.into_raw_parts().0);+ descriptions.write(description_values.into_raw_parts().0);+ data_types.write(type_values.into_raw_parts().0); }
ReadRawat Lines 349-350 already uses this order.🤖 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 `@opc_hda_bindings/src/server/mod.rs` around lines 126 - 132, Update the output-building logic in the affected method and the analogous GetAggregates and GetHistorianStatus methods so every builder’s finish() result is collected successfully before writing any caller output pointer, matching the ordering used by ReadRaw. Only after all finishes succeed should the unsafe block assign count and the array pointers, preserving the existing error propagation without partially initialized out-params.
669-699: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the partial-failure path.
The test exercises only successful results. The riskiest code in this file is the failure path:
push_hda_itemwith a default item at Line 344, theHdaItemCleanuprollback, and theS_FALSEstatus frombatch_status. Add a service variant that returnsHdaServerItemResult::failurefor one handle, then assert that the client reports the per-item error and that the successful item still decodes.Also note that
HdaService::write_valueshas no COM caller in this adapter and no test. Confirm that an update interface is planned, or remove the method until it is used.🤖 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 `@opc_hda_bindings/src/server/mod.rs` around lines 669 - 699, Extend client_and_server_adapters_round_trip_nested_task_memory with a service variant returning HdaServerItemResult::failure for one handle, then verify the client exposes that per-item error while the other item still decodes successfully, covering push_hda_item, HdaItemCleanup rollback, and batch_status returning S_FALSE. Also resolve the unused HdaService::write_values method by confirming an update interface is planned or removing it until a COM caller exists.opc_classic_utils/examples/ownership.rs (1)
9-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe example stores Rust
Stringvalues in COM task memory.The comment on Line 9 states that COM outputs use the task allocator. A Rust
Stringis not an ABI type and never crosses a COM boundary. The crate documentation inopc_classic_utils/src/memory/array.rsLines 40-42 restrictsDropElementstoVARIANTand to structures whose Rust drop glue matches the COM cleanup contract.opc_classic_utils/README.mdLines 31-33 directs readers toFreePwstrElementsfor string arrays.Use
OwnedPwstrwithFreePwstrElementsso the example matches the documented ownership model.♻️ Proposed fix
-use opc_classic_utils::{CoTaskMemArrayBuilder, DropElements, WideCString};+use opc_classic_utils::{CoTaskMemArrayBuilder, FreePwstrElements, OwnedPwstr, WideCString}; fn main() -> windows_core::Result<()> { // COM input parameters borrow ordinary Rust-owned memory. let item_id = WideCString::try_from("Channel.Device.Tag").expect("the item id must not contain NUL"); println!("input pointer: {:?}", item_id.as_pcwstr()); // COM outputs use the task allocator and an explicit element cleanup policy. - let mut values = CoTaskMemArrayBuilder::new(3, DropElements)?;- values.push(String::from("one")).unwrap();- values.push(String::from("two")).unwrap();- values.push(String::from("three")).unwrap();+ let mut values = CoTaskMemArrayBuilder::new(3, FreePwstrElements)?;+ for text in ["one", "two", "three"] {+ let owned = OwnedPwstr::new(text)?;+ values.push(owned.into_raw()).expect("capacity is reserved");+ } let values = values.finish()?; - println!("values: {:?}", values.as_slice());+ println!("{} task-allocated strings", values.len()); Ok(()) }🤖 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 `@opc_classic_utils/examples/ownership.rs` around lines 9 - 15, Update the example around CoTaskMemArrayBuilder to store OwnedPwstr values instead of Rust String values, and use FreePwstrElements as the element cleanup policy. Preserve the existing three-string example output while matching the documented COM ownership model.opc_classic_utils/src/server.rs (1)
15-17: 🩺 Stability & Availability | 🔵 TrivialConsider recording the panic before returning
E_UNEXPECTED.
catch_ffidiscards the panic payload. Every panic in a COM method becomes an opaqueE_UNEXPECTEDat the client. A COM server has no other channel to report the failure. Log the payload, or emit a trace event, before you map the panic to an HRESULT.🤖 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 `@opc_classic_utils/src/server.rs` around lines 15 - 17, Update catch_ffi to capture the panic payload from catch_unwind and record it through the server’s existing logging or tracing mechanism before returning Error::from_hresult(E_UNEXPECTED); preserve the current HRESULT mapping and successful Result behavior.opc_classic_utils/src/memory/wide.rs (1)
136-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a round-trip test for
OwnedPwstr.The tests cover
WideCStringonly.OwnedPwstrperforms theCoTaskMemAllocandCoTaskMemFreecalls and holds the raw pointer. Add a test that builds a value withnew, reads it back withto_string_lossy, and re-adopts it throughinto_rawandfrom_raw.♻️ Proposed test
#[test] fn wide_c_string_is_terminated() { let value = WideCString::try_from("OPC").unwrap(); assert_eq!( value.as_slice_with_nul(), &[b'O' as u16, b'P' as u16, b'C' as u16, 0] ); } ++ #[test]+ fn owned_pwstr_round_trips() {+ let value = OwnedPwstr::new("OPC").unwrap();+ assert_eq!(value.to_string_lossy(), "OPC");+ let raw = value.into_raw();+ let value = unsafe { OwnedPwstr::from_raw(raw.0) };+ assert_eq!(value.to_string_lossy(), "OPC");+ }++ #[test]+ fn owned_pwstr_null_is_empty() {+ let value = unsafe { OwnedPwstr::from_raw(std::ptr::null_mut()) };+ assert!(value.is_null());+ assert_eq!(value.to_string_lossy(), "");+ }🤖 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 `@opc_classic_utils/src/memory/wide.rs` around lines 136 - 153, Add a test in the #[cfg(test)] mod tests covering OwnedPwstr: construct it with new, verify the content via to_string_lossy, transfer ownership with into_raw, then re-adopt the pointer using from_raw and verify the round-tripped string before cleanup.opc_comn_bindings/src/client/mod.rs (1)
156-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the wide-string conversion helper.
Lines 47-48, 140-141, and 157-158 repeat the same
WideCString::try_from(...).map_err(...)conversion with an inline fully qualifiedE_INVALIDARG. Extract one private helper and call it from all three sites.♻️ Proposed helper
fnwide(value:&str) -> Result<WideCString>{WideCString::try_from(value).map_err(|_| Error::from_hresult(windows::Win32::Foundation::E_INVALIDARG))}🤖 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 `@opc_comn_bindings/src/client/mod.rs` around lines 156 - 160, Extract a private wide-string conversion helper, such as wide, that performs the shared WideCString::try_from conversion and maps failures to E_INVALIDARG. Replace the duplicated inline conversions in the methods at the three referenced sites, including request, with calls to this helper while preserving their existing Result behavior.opc_comn_bindings/src/server/mod.rs (1)
306-316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for
GuidEnumeratorServer.The only test covers
CommonServer.GuidEnumeratorServerholds the most intricate logic in this file: the position mutex, the partial-fetchS_FALSEreturn, and theClonesnapshot. None of it is exercised. Add a round trip throughGuidEnumeratorthat reads a full batch, then a partial batch, then verifiesresetandtry_clonecursor behavior.💚 Proposed test
#[test]fnguid_enumerator_round_trips_batches(){usecrate::client::GuidEnumerator;let a = GUID::from_u128(1);let b = GUID::from_u128(2);let c = GUID::from_u128(3);let raw:IOPCEnumGUID = GuidEnumeratorServer::new(vec![a, b, c]).into();let enumerator = GuidEnumerator::new(raw);assert_eq!(enumerator.next_batch(2).unwrap(),[a, b]);let snapshot = enumerator.try_clone().unwrap();assert_eq!(enumerator.next_batch(2).unwrap(),[c]);assert!(enumerator.next_batch(2).unwrap().is_empty());// The clone keeps the cursor position it was taken at.assert_eq!(snapshot.next_batch(2).unwrap(),[c]); enumerator.reset().unwrap();assert_eq!(enumerator.next_batch(3).unwrap(),[a, b, c]);}🤖 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 `@opc_comn_bindings/src/server/mod.rs` around lines 306 - 316, Add a `guid_enumerator_round_trips_batches` test alongside `common_adapter_round_trips_task_memory` that constructs `GuidEnumeratorServer` with three GUIDs and wraps it in `GuidEnumerator`; verify a full batch, the partial final batch, an empty exhausted batch, and the clone’s preserved cursor, then reset the original and verify all GUIDs are returned again.
🤖 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 `@opc_ae_bindings/src/server/mod.rs`:
- Around line 201-206: Move all fallible allocation calls before the unsafe
out-parameter writes: in opc_ae_bindings/src/server/mod.rs lines 201-206, finish
id_values, description_values, and type_values before writing count, ids,
descriptions, and data_types together; likewise at lines 312-315, finish strings
before writing count and output together.
- Around line 147-169: Validate the client-supplied pointers at the start of
QuerySubConditionNames and QuerySourceConditions before calling to_string();
when either PCWSTR is null, return E_INVALIDARG through the existing catch_ffi
error path, and preserve the current decoding and write_strings behavior for
non-null inputs.
In `@opc_classic_utils/README.md`:
- Around line 14-27: Update the README Rust example around the CoTaskMemOut
usage so it no longer relies on the hidden-line marker for error handling. Wrap
the snippet in an example function returning windows_core::Result<()> and
replace the hidden `# Ok...` line with a normal `Ok(())` before the closing code
fence.
In `@opc_classic_utils/src/memory/array.rs`:
- Around line 260-294: Make the drop counter test-local in both
builder_rolls_back_initialized_prefix and finished_array_drops_all_elements
instead of using the shared static DROPS. Add std::sync::Arc in the test module,
create an independent counter per test, and have each CountDrop instance update
its test’s counter so parallel execution cannot interfere.
In `@opc_classic_utils/src/server.rs`:
- Around line 74-90: Update CreateInstance to validate the requested iid and
handle object.query as a Result rather than discarding its error: only perform
the query when iid refers to IUnknown, and propagate any query failure instead
of returning success. Preserve output initialization and existing aggregation
validation, while ensuring activation and query ordering does not bypass these
checks.
In `@opc_comn_bindings/src/server/mod.rs`:
- Around line 76-81: Guard the PCWSTR inputs before conversion in SetClientName
and the two corresponding server COM entry points at
opc_comn_bindings/src/server/mod.rs lines 76-81, 244-249, and 269-274; return
E_POINTER for null pointers, or reuse a shared safe-wrapper helper, and only
call PCWSTR::to_string after validation.
In `@opc_da_bindings/src/server/mod.rs`:
- Around line 295-389: Initialize the relevant COM out-parameters to null at the
beginning of AddItems, ValidateItems, RemoveItems, SetActiveState,
SetClientHandles, SetDatatypes, and Write, before entering operations that can
fail. Preserve the existing result-writing behavior while ensuring every early
error return leaves results and errors safely initialized, matching the existing
Read pattern.
In `@opc_hda_bindings/src/server/mod.rs`:
- Around line 265-284: Update ValidateItemIDs to initialize the errors output
through initialize_output before calling decode_strings, ensuring it is valid
even when input validation fails; retain or safely deduplicate the later
initialization in write_unit_results.
- Around line 351-357: Update the unsafe `OPCHDA_TIME` handling for `start` and
`end` so any caller-owned `szTime` buffer is freed before clearing it when
`bString` is true; otherwise preserve the pointer. Ensure the cleanup occurs
before setting `bString` to false and retain the existing resolved `ftTime`
assignments.
- Around line 500-516: Add null-pointer validation before wide-string conversion
in decode_strings and decode_time: reject each null PCWSTR item and a null
szTime when bString is true by returning E_POINTER, then call to_string only for
non-null pointers.
---
Nitpick comments:
In `@opc_ae_bindings/src/client/mod.rs`:
- Around line 74-83: Rename the second `areas` and `sources` bindings in the
`WideCString::as_pcwstr` conversion to distinct pointer-vector names, while
keeping the original owned `WideCString` vectors unchanged and alive for the
pointer usage that follows.
- Around line 370-384: Update condition_names to delegate to the existing
query_string_array helper instead of duplicating the QueryConditionNames,
CoTaskMemOut, and PWSTR conversion logic. Match the usage pattern in
subcondition_names and source_conditions while preserving the current
event_category argument and Result<Vec<String>> behavior.
- Around line 522-528: Update pwstr_string to decode non-null PWSTR values
lossily instead of calling to_string().unwrap_or_default(), preserving readable
content when the UTF-16 data is invalid while retaining the empty result for
null pointers.
In `@opc_ae_bindings/src/server/mod.rs`:
- Around line 283-300: Create a shared crate module containing the StatusCleanup
type and its Cleanup implementation for __MIDL___MIDL_itf_opc_ae_0000_0001_0005,
preserving the existing vendor-string cleanup behavior. In
opc_ae_bindings/src/server/mod.rs#L283-L300, remove the local definition and
import the shared implementation; in
opc_ae_bindings/src/client/mod.rs#L275-L292, delete the duplicate and import the
same shared implementation.
- Around line 399-415: Extend client_and_server_adapters_round_trip_task_memory
and TestService so enable_area records its decoded names, then assert that
enabling "plant" reaches the service unchanged. Add coverage for
source_conditions, disable_areas, enable_sources, and disable_sources, and
exercise the remaining adapter methods that are expected to return E_NOTIMPL,
asserting those HRESULTs explicitly.
In `@opc_classic_utils/examples/ownership.rs`:
- Around line 9-15: Update the example around CoTaskMemArrayBuilder to store
OwnedPwstr values instead of Rust String values, and use FreePwstrElements as
the element cleanup policy. Preserve the existing three-string example output
while matching the documented COM ownership model.
In `@opc_classic_utils/src/memory/wide.rs`:
- Around line 136-153: Add a test in the #[cfg(test)] mod tests covering
OwnedPwstr: construct it with new, verify the content via to_string_lossy,
transfer ownership with into_raw, then re-adopt the pointer using from_raw and
verify the round-tripped string before cleanup.
In `@opc_classic_utils/src/server.rs`:
- Around line 15-17: Update catch_ffi to capture the panic payload from
catch_unwind and record it through the server’s existing logging or tracing
mechanism before returning Error::from_hresult(E_UNEXPECTED); preserve the
current HRESULT mapping and successful Result behavior.
In `@opc_comn_bindings/src/client/mod.rs`:
- Around line 156-160: Extract a private wide-string conversion helper, such as
wide, that performs the shared WideCString::try_from conversion and maps
failures to E_INVALIDARG. Replace the duplicated inline conversions in the
methods at the three referenced sites, including request, with calls to this
helper while preserving their existing Result behavior.
In `@opc_comn_bindings/src/server/mod.rs`:
- Around line 306-316: Add a `guid_enumerator_round_trips_batches` test
alongside `common_adapter_round_trips_task_memory` that constructs
`GuidEnumeratorServer` with three GUIDs and wraps it in `GuidEnumerator`; verify
a full batch, the partial final batch, an empty exhausted batch, and the clone’s
preserved cursor, then reset the original and verify all GUIDs are returned
again.
In `@opc_da_bindings/src/client/group.rs`:
- Around line 107-246: Extract the duplicated ID/path conversion and
tagOPCITEMDEF construction from add_items and validate_items into a shared
build_definitions helper, and extract their identical result/error-to-AddedItem
mapping into map_item_results. Update both methods to use these helpers while
preserving their distinct COM calls, and use the shared definitions length
consistently when passing the count argument.
In `@opc_da_bindings/src/server/mod.rs`:
- Around line 880-901: Extend client_and_server_adapters_round_trip_owned_values
with a TestService variant that returns ServerItemResult::failure for one item
while preserving a successful result for another; exercise batch_status and
Write, assert the failed entry maps to Err(ItemError { .. }) while successful
entries decode, and verify blob cleanup through ItemResultCleanup.
In `@opc_hda_bindings/src/server/mod.rs`:
- Around line 126-132: Update the output-building logic in the affected method
and the analogous GetAggregates and GetHistorianStatus methods so every
builder’s finish() result is collected successfully before writing any caller
output pointer, matching the ordering used by ReadRaw. Only after all finishes
succeed should the unsafe block assign count and the array pointers, preserving
the existing error propagation without partially initialized out-params.
- Around line 669-699: Extend
client_and_server_adapters_round_trip_nested_task_memory with a service variant
returning HdaServerItemResult::failure for one handle, then verify the client
exposes that per-item error while the other item still decodes successfully,
covering push_hda_item, HdaItemCleanup rollback, and batch_status returning
S_FALSE. Also resolve the unused HdaService::write_values method by confirming
an update interface is planned or removing it until a COM caller exists.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aef2c98c-a8bf-44f3-9e6c-9a32a28db20d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (55)
Cargo.tomlREADME.mdopc_ae_bindings/Cargo.tomlopc_ae_bindings/README.mdopc_ae_bindings/src/bindings.rsopc_ae_bindings/src/client/mod.rsopc_ae_bindings/src/lib.rsopc_ae_bindings/src/server/mod.rsopc_classic_utils/Cargo.tomlopc_classic_utils/README.mdopc_classic_utils/examples/array_functions.rsopc_classic_utils/examples/basic_usage.rsopc_classic_utils/examples/convenience_functions.rsopc_classic_utils/examples/memory_management_comparison.rsopc_classic_utils/examples/opc_scenarios.rsopc_classic_utils/examples/ownership.rsopc_classic_utils/examples/transparent_repr_demo.rsopc_classic_utils/src/com.rsopc_classic_utils/src/lib.rsopc_classic_utils/src/memory/array.rsopc_classic_utils/src/memory/mod.rsopc_classic_utils/src/memory/out.rsopc_classic_utils/src/memory/ptr.rsopc_classic_utils/src/memory/ptr_array.rsopc_classic_utils/src/memory/tests.rsopc_classic_utils/src/memory/wide.rsopc_classic_utils/src/memory/wstring.rsopc_classic_utils/src/server.rsopc_comn_bindings/Cargo.tomlopc_comn_bindings/README.mdopc_comn_bindings/src/bindings.rsopc_comn_bindings/src/client/mod.rsopc_comn_bindings/src/lib.rsopc_comn_bindings/src/server/mod.rsopc_da/Cargo.tomlopc_da/README.mdopc_da/src/lib.rsopc_da_bindings/Cargo.tomlopc_da_bindings/README.mdopc_da_bindings/build.rsopc_da_bindings/src/bindings.rsopc_da_bindings/src/client/group.rsopc_da_bindings/src/client/mod.rsopc_da_bindings/src/client/properties.rsopc_da_bindings/src/client/types.rsopc_da_bindings/src/lib.rsopc_da_bindings/src/server/mod.rsopc_hda_bindings/Cargo.tomlopc_hda_bindings/README.mdopc_hda_bindings/build.rsopc_hda_bindings/src/bindings.rsopc_hda_bindings/src/client/mod.rsopc_hda_bindings/src/lib.rsopc_hda_bindings/src/server/mod.rsrust-toolchain.toml
💤 Files with no reviewable changes (13)
- opc_da/README.md
- opc_classic_utils/examples/convenience_functions.rs
- opc_classic_utils/examples/basic_usage.rs
- opc_classic_utils/examples/opc_scenarios.rs
- opc_classic_utils/examples/transparent_repr_demo.rs
- opc_classic_utils/examples/array_functions.rs
- opc_da/Cargo.toml
- opc_classic_utils/src/memory/ptr_array.rs
- opc_da/src/lib.rs
- opc_classic_utils/src/memory/tests.rs
- opc_classic_utils/src/memory/wstring.rs
- opc_classic_utils/examples/memory_management_comparison.rs
- opc_classic_utils/src/memory/ptr.rs
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
opc_da_bindings/src/client/mod.rs (1)
117-127: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRelease the server-side group when
AddGroupreturns no object.
AddGroupsucceeded, so the server already created the group and returnedserver_handle. The error at Line 120 discards that handle, so the group stays allocated on the server for the rest of the session. CallRemoveGroup(server_handle, true)before you return the error.🐛 Proposed fix
- let object =- object.ok_or_else(|| Error::unexpected("AddGroup returned no group object"))?;+ let Some(object) = object else {+ // The server created the group, so release it before reporting the failure.+ let _ = unsafe { self.inner.RemoveGroup(server_handle, true) };+ return Err(Error::unexpected("AddGroup returned no group object"));+ };🤖 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 `@opc_da_bindings/src/client/mod.rs` around lines 117 - 127, Update the `object.ok_or_else` handling in the `AddGroup` flow to call `RemoveGroup` with `server_handle` and forced removal before returning the unexpected-error result when no group object is returned. Preserve the existing successful `DaGroup::new` path and propagate the original missing-object error.opc_da_bindings/src/server/mod.rs (1)
213-231: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winInsert the group into
groupsonly after the out-parameters are written.
AddGroupinserts the entry at Line 217, then writesoutputat Line 224. Ifoutput.writefails, the method returns an error, but the group entry stays inself.groups. The client believes no group exists, andGetGroupByNamecan still return the orphaned group. Move the insert after the successful writes, or remove the entry on the error path.🤖 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 `@opc_da_bindings/src/server/mod.rs` around lines 213 - 231, The AddGroup flow currently inserts the GroupEntry before output writes can fail, leaving an orphaned group on error. Move the self.groups insertion until after output.write, server_handle.write, and revised_update_rate.write complete successfully, preserving the existing error propagation and group data.
🧹 Nitpick comments (17)
opc_classic_types/src/value.rs (1)
62-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering the remaining scalar types in the
Fromimpls.The
Fromimpls coveri32,f64,bool,String, and&str, but notu32,i16,u16,i64,u64,f32, orVec<u8>. Callers that build values generically must therefore mixValue::fromwith direct variant construction. A declarative macro can generate the whole set and keep the surface symmetric.🤖 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 `@opc_classic_types/src/value.rs` around lines 62 - 90, Extend the From implementations for Value to cover u32, i16, u16, i64, u64, f32, and Vec<u8>, mapping each input to its corresponding Value variant. Use a declarative macro to generate the scalar and existing conversions consistently, while preserving the current String and &str ownership behavior.opc_classic_types/src/error.rs (1)
152-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the unit tests for the new foundational types.
This crate is the shared contract for every bindings crate, and one test covers only the sign bit. Add tests for
Error::from_codekind mapping, and for theTimestamp::from_system_time/to_system_timeround trip.com.rshas no test module; theGuidDebugformat is a good pure-Rust candidate there.♻️ Proposed additional tests
#[test] fn failure_uses_signed_status_bit() { assert!(ErrorCode::INVALID_ARGUMENT.is_failure()); assert!(ErrorCode::OK.is_success()); } ++ #[test]+ fn from_code_maps_known_kinds() {+ assert_eq!(+ Error::from_code(ErrorCode::NULL_POINTER).kind(),+ ErrorKind::NullPointer+ );+ assert_eq!(+ Error::from_code(ErrorCode::TYPE_MISMATCH).kind(),+ ErrorKind::Other+ );+ }++ #[test]+ fn constructors_keep_code_and_kind_consistent() {+ let error = Error::out_of_memory("allocation failed");+ assert_eq!(error.code(), ErrorCode::OUT_OF_MEMORY);+ assert_eq!(error.kind(), ErrorKind::OutOfMemory);+ assert_eq!(error.message(), "allocation failed");+ }🤖 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 `@opc_classic_types/src/error.rs` around lines 152 - 161, Extend the #[cfg(test)] coverage in the foundational types: add assertions for Error::from_code kind mapping, verify Timestamp::from_system_time and to_system_time round-trip consistently, and create a com.rs test module covering the Guid Debug format. Keep the existing failure/success status-bit test unchanged.opc_classic_utils/src/server.rs (1)
163-189: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider making the
REGCLSflags configurable alongside the class context.
register_in_contextparameterizesCLSCTXbut hardcodesREGCLS_MULTIPLEUSE. A single-use or suspended registration therefore cannot be expressed. If onlyREGCLS_MULTIPLEUSEis intended for now, state that in a doc comment so the constraint is explicit.🤖 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 `@opc_classic_utils/src/server.rs` around lines 163 - 189, Update register_in_context to accept a configurable REGCLS value alongside ClassContext and pass it to CoRegisterClassObject, enabling single-use or suspended registrations. If the API must remain fixed to REGCLS_MULTIPLEUSE, add a doc comment to register_in_context explicitly documenting that limitation.opc_classic_utils/src/memory/out.rs (1)
165-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the
into_arraytransfer path.The test proves the unadopted-drop path. The adopted path is the one every client uses, and it is only covered indirectly by the round-trip tests that need Windows COM. Add a pure-Rust test so
into_arraycleans up exactly once, and soset_lenis exercised.♻️ Proposed additional test
#[test] fn array_output_deep_cleans_when_not_adopted() { let drops = Arc::new(AtomicUsize::new(0)); let mut builder = CoTaskMemArrayBuilder::new(2, DropElements).unwrap(); builder.push(CountDrop(drops.clone())).ok().unwrap(); builder.push(CountDrop(drops.clone())).ok().unwrap(); let (ptr, len) = builder.finish().unwrap().into_raw_parts(); let mut output = CoTaskMemArrayOut::<CountDrop, _>::new(len, DropElements); unsafe { output.as_mut_ptr().write(ptr) }; drop(output); assert_eq!(drops.load(Ordering::Relaxed), 2); } ++ #[test]+ fn adopted_array_output_cleans_up_exactly_once() {+ let drops = Arc::new(AtomicUsize::new(0));+ let mut builder = CoTaskMemArrayBuilder::new(2, DropElements).unwrap();+ builder.push(CountDrop(drops.clone())).ok().unwrap();+ builder.push(CountDrop(drops.clone())).ok().unwrap();+ let (ptr, len) = builder.finish().unwrap().into_raw_parts();++ // Model a call that reports its element count only on return.+ let mut output = CoTaskMemArrayOut::<CountDrop, _>::new(0, DropElements);+ unsafe { output.as_mut_ptr().write(ptr) };+ unsafe { output.set_len(len) };+ let array = unsafe { output.into_array() }.unwrap();+ assert_eq!(array.len(), 2);+ assert_eq!(drops.load(Ordering::Relaxed), 0);+ drop(array);++ assert_eq!(drops.load(Ordering::Relaxed), 2);+ }🤖 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 `@opc_classic_utils/src/memory/out.rs` around lines 165 - 179, Add a pure-Rust test alongside array_output_deep_cleans_when_not_adopted that constructs a CoTaskMemArrayOut with tracked CountDrop elements, writes the allocated pointer, calls set_len, and transfers ownership through into_array. Drop the resulting array and assert the drop counter reports exactly one cleanup per element, covering the adopted transfer path and set_len behavior.opc_classic_utils/src/memory/array.rs (1)
255-321: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe per-test drop counter is now in place; add two more transactional tests.
The earlier concern about the process-global
DROPScounter is resolved. Each test now owns anArc<AtomicUsize>, so parallel execution cannot interfere.Two core guarantees remain untested:
into_raw_partsmust not run element cleanup, andpushmust returnErr(value)and give ownership back when the builder is full.♻️ Proposed additional tests
#[test] fn zero_length_array_is_supported() { let builder = CoTaskMemArrayBuilder::<u32, _>::new(0, NoCleanup).unwrap(); let array = builder.finish().unwrap(); assert!(array.is_empty()); assert!(array.as_ptr().is_null()); } ++ #[test]+ fn into_raw_parts_transfers_element_ownership() {+ let drops = Arc::new(AtomicUsize::new(0));+ let mut builder = CoTaskMemArrayBuilder::new(1, DropElements).unwrap();+ builder+ .push(CountDrop {+ drops: drops.clone(),+ })+ .ok()+ .unwrap();+ let (ptr, len) = builder.finish().unwrap().into_raw_parts();+ assert_eq!(drops.load(Ordering::Relaxed), 0);++ // Reclaim so the test does not leak.+ let array =+ unsafe { CoTaskMemArray::from_raw_parts(ptr, len, DropElements) }.unwrap();+ drop(array);+ assert_eq!(drops.load(Ordering::Relaxed), 1);+ }++ #[test]+ fn push_returns_the_value_when_full() {+ let mut builder = CoTaskMemArrayBuilder::<u32, _>::new(1, NoCleanup).unwrap();+ builder.push(1).ok().unwrap();+ assert_eq!(builder.push(2), Err(2));+ }🤖 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 `@opc_classic_utils/src/memory/array.rs` around lines 255 - 321, Extend the existing tests for CoTaskMemArrayBuilder with two transactional cases: verify that finishing via into_raw_parts does not clean up initialized elements, and verify that pushing into a full builder returns Err(value) with the original value still owned by the caller. Reuse the per-test Arc<AtomicUsize> drop counter and existing CountDrop/DropElements helpers, and assert the expected cleanup counts and returned value.opc_ae_bindings/src/server/mod.rs (1)
441-459: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the round trip test to the rejection paths.
The test covers the success paths only. Add assertions for the paths that the adapter validates or refuses:
client.source_conditions("plant"),client.disable_areas,client.enable_sources, andclient.disable_sources, which exercise the remainingdecode_stringsandenable_*branches.client.create_subscription(...)andclient.area_browser(), which must return thenot_implementederror rather than succeed.- A service that returns
Err(...)fromstatus(), to confirm the error code survivesto_abi_errorandfrom_abi_error.🤖 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 `@opc_ae_bindings/src/server/mod.rs` around lines 441 - 459, The round-trip test client_and_server_round_trip_without_public_windows_types must cover the adapter’s rejection and error-propagation paths. Add assertions for source_conditions, disable_areas, enable_sources, and disable_sources, verify create_subscription and area_browser return the not_implemented error, and add a service whose status() returns Err(...) to confirm the resulting error code survives ABI conversion.opc_ae_bindings/src/client/mod.rs (1)
527-534: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove the redundant QueryInterface round trip.
CreateEventSubscriptionis requested withIOPCEventSubscriptionMgt::IID, but the generated wrapper returnswindows_core::IUnknown. Use the returned interface directly instead of wrapping it inComObjectand callingQueryInterfaceagain. This avoids an extra AddRef/Release pair and removes one possibleQueryInterfacefailure path on a broken AE server.🤖 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 `@opc_ae_bindings/src/client/mod.rs` around lines 527 - 534, Update the event subscription construction around CreateEventSubscription to use the returned IUnknown directly, removing the object_from_interface conversion and subsequent interface_from_object QueryInterface call. Preserve the null-response error handling and populate EventSubscription.inner from the returned interface without introducing another COM round trip.opc_ae_bindings/src/abi.rs (1)
32-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the diagnostic text when converting Rust errors to ABI errors.
service_call()returns every service error throughto_abi_error(), butfrom_hresult()drops theErrormessage. Build the ABI error withAbiError::new(..., error.message())so in-process callers do not lose diagnostic text.🤖 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 `@opc_ae_bindings/src/abi.rs` around lines 32 - 34, Update to_abi_error to construct the AbiError with the HRESULT from error.code().raw() and the original error.message(), using AbiError::new instead of AbiError::from_hresult, so service_call preserves diagnostic text for in-process callers.opc_comn_bindings/src/server/mod.rs (1)
419-437: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the argument assertions out of the service methods.
These
assert_eq!calls run insideabi_boundary, which usescatch_abi. A panic is converted intoE_UNEXPECTED, so a mismatch surfaces on the client as an opaque error and the assertion message is lost. Record the received arguments in shared state, and assert on that state in the test body.♻️ Sketch for `TestServerList::enum_classes`
- struct TestServerList;+ #[derive(Default)]+ struct TestServerList {+ implemented: Mutex<Vec<Guid>>,+ } impl ServerListService for TestServerList { fn enum_classes(&self, implemented: &[Guid], required: &[Guid]) -> Result<Vec<Guid>> { - assert_eq!(implemented, [CLASS_ID]);- assert!(required.is_empty());+ *self.implemented.lock().unwrap() = implemented.to_vec();+ if !required.is_empty() {+ return Err(Error::invalid_argument("unexpected required categories"));+ } Ok(vec![CLASS_ID]) }Also applies to: 464-468
🤖 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 `@opc_comn_bindings/src/server/mod.rs` around lines 419 - 437, Remove the argument assertions from the service methods enum_classes, class_details, and class_id_from_prog_id; instead, record each received argument in the existing shared test state and assert the expected values in the test body after the ABI call completes, preserving the methods’ current return values.opc_comn_bindings/src/abi.rs (1)
11-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCarry the error message through
to_abi_error.
from_abi_errorpreserves the error code and message, butto_abi_errormaps Rust errors to an HRESULT-only ABI error. Build the ABI error withAbiError::new(windows_core::HRESULT(error.code().raw()), error.message())so Rust-backed server errors keep their original text.🤖 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 `@opc_comn_bindings/src/abi.rs` around lines 11 - 13, Update to_abi_error to construct the AbiError with AbiError::new, passing the existing HRESULT derived from error.code().raw() and error.message() so both the error code and original message are preserved.opc_da_bindings/src/server/mod.rs (1)
820-827: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueHandle counter wraparound instead of failing at one value.
next_nonzerorejects only0andu32::MAX. After the counter wraps, it restarts at1and can return a handle that is still in use by a live group. Check thegroupsmap for a collision, or fail permanently once the counter wraps.🤖 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 `@opc_da_bindings/src/server/mod.rs` around lines 820 - 827, Update next_nonzero to handle AtomicU32 wraparound safely: prevent returning a value already present in the live groups map, or permanently fail once wraparound is detected. Preserve the existing rejection of invalid counter values and ensure generated handles remain unique among active groups.opc_hda_bindings/src/client/mod.rs (3)
360-365: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider caching the
IOPCHDA_SyncReadinterface.
read_rawperforms aComObjectclone and a QueryInterface on every call. For DCOM servers the QueryInterface is a round trip. Resolve the interface once at construction, or memoize it inHdaClient.🤖 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 `@opc_hda_bindings/src/client/mod.rs` around lines 360 - 365, Update HdaClient to cache the IOPCHDA_SyncRead interface during construction or lazy initialization, then reuse that cached interface in read_raw instead of cloning self.object() and calling interface_from_object on every invocation. Preserve the existing read behavior and error propagation.
531-533: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOne u32 count conversion is defined twice in the crate. The client and the server each declare their own helper with identical logic and identical error text, so the two copies can drift.
opc_hda_bindings/src/client/mod.rs#L531-L533: keep thiscounthelper, or move it tocrate::convert, and remove the private duplicate.opc_hda_bindings/src/server/mod.rs#L597-L599: deletechecked_countand call the shared helper instead.🤖 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 `@opc_hda_bindings/src/client/mod.rs` around lines 531 - 533, The u32 count conversion is duplicated between the client and server modules. In opc_hda_bindings/src/client/mod.rs lines 531-533, retain or move count into crate::convert as the shared helper; in opc_hda_bindings/src/server/mod.rs lines 597-599, remove checked_count and update its callers to use the shared count helper, preserving the existing error behavior.
523-529: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
pwstr_stringhides malformed server strings.Both a null pointer and invalid UTF-16 produce an empty String, so an attribute or aggregate silently loses its name. opc_da_bindings/src/client/group.rs lines 94-99 return
Error::invalid_argumentfor the same condition. Align the two crates, or useto_string_lossyso the caller still sees the recoverable text.🤖 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 `@opc_hda_bindings/src/client/mod.rs` around lines 523 - 529, Update pwstr_string to stop silently converting invalid UTF-16 to an empty String: preserve null-pointer handling, but propagate the conversion failure as the crate’s established invalid-argument error (matching opc_da_bindings), or use a lossy conversion that retains recoverable text instead of defaulting to empty.opc_hda_bindings/src/server/mod.rs (2)
378-390: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReport a sample conversion failure per item instead of failing the batch.
build_hda_itemcallsvalue_to_abi, which returnsnot_implementedforValue::BytesandValue::Array. Line 381 propagates that failure with?, so one unsupported sample value discards every other item in the read and returnsE_NOTIMPLwith null out-params. Convert the failure into a per-item HRESULT, as theErr(error)arm already does.♻️ Proposed refactor
for item in result.items { match item.result { - Ok(item) => {- push_hda_item(&mut values, build_hda_item(item)?)?;- push(&mut item_errors, HRESULT(0))?;- }+ Ok(item) => match build_hda_item(item) {+ Ok(item) => {+ push_hda_item(&mut values, item)?;+ push(&mut item_errors, HRESULT(0))?;+ }+ Err(error) => {+ has_error = true;+ push_hda_item(&mut values, tagOPCHDA_ITEM::default())?;+ push(&mut item_errors, HRESULT(error.code().raw()))?;+ }+ }, Err(error) => {🤖 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 `@opc_hda_bindings/src/server/mod.rs` around lines 378 - 390, Handle failures from build_hda_item within the per-item loop instead of propagating them with ?. Update the Ok(item) arm to convert conversion errors into the corresponding item HRESULT, append the default tagOPCHDA_ITEM value, and set has_error, matching the existing Err(error) behavior while allowing the remaining samples to be processed.
724-755: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the round-trip test to the failure paths.
The test covers only successful results. Add cases for
HdaTime::Expression, an item that returnsHdaServerItemResult::failure, andvalidate_item_ids. These paths exercise the partial-success mapping and the nestedtagOPCHDA_ITEMcleanup, which the current test never reaches.🤖 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 `@opc_hda_bindings/src/server/mod.rs` around lines 724 - 755, The round-trip test client_and_server_adapters_round_trip_nested_task_memory currently covers only successful operations; extend it to exercise failure mappings and cleanup. Add assertions for HdaTime::Expression, an item producing HdaServerItemResult::failure, and validate_item_ids, verifying the expected partial-success/error results and nested tagOPCHDA_ITEM cleanup while preserving the existing success assertions.opc_hda_bindings/src/convert.rs (1)
45-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the VARIANT conversion helpers into a shared crate.
opc_hda_bindings/src/convert.rsandopc_da_bindings/src/abi.rseach definevalue_to_abi,value_from_abi, andscalar_variantwith the same Automation mapping and tests. Put these helpers inopc_classic_utilsand have the binding crates re-export them so the conversion layer cannot drift between DA, AE, and HDA.🤖 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 `@opc_hda_bindings/src/convert.rs` around lines 45 - 122, Move value_to_abi, value_from_abi, scalar_variant, and their shared tests from the binding-specific conversion modules into opc_classic_utils, preserving the existing Automation mappings and error behavior. Update the DA and HDA modules to re-export the shared helpers instead of defining local copies, and ensure AE uses the same shared conversion layer so all binding crates rely on one implementation.
🤖 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 `@opc_ae_bindings/src/client/mod.rs`:
- Around line 381-384: Update the GetStatus handling around into_array to use a
normal COM output pointer for the returned status struct, validate it for null,
and return Error::null_pointer(...) when absent. Preserve the existing ABI error
propagation and status/value extraction only after successful pointer
validation, with proper ownership cleanup for the COM output.
In `@opc_classic_utils/src/server.rs`:
- Around line 208-210: Update each server ABI module’s to_abi_error conversion,
including opc_classic_utils and the corresponding functions in opc_da_bindings,
opc_hda_bindings, and opc_comn_bindings, to validate error.code().raw() with
ErrorCode::is_failure before constructing the Windows HRESULT. Ensure
success-valued service error codes are converted to an appropriate failure
HRESULT while preserving existing handling for failure codes.
In `@opc_comn_bindings/src/server/mod.rs`:
- Around line 131-142: Update Next so a null fetched pointer is accepted when
count equals 1: avoid unconditionally calling initialize_output(fetched),
preserve validation for other count values, and only initialize/write fetched
when it is non-null while retaining the existing values pointer checks and
result behavior.
In `@opc_da_bindings/README.md`:
- Line 3: Update the src/bindings.rs description in opc_da_bindings/README.md
lines 3-3 and opc_hda_bindings/README.md lines 3-3 to use a clear compound
modifier, such as “contains private generated code for the unsafe ABI” or
“unsafe-ABI,” while preserving the surrounding client-module description.
In `@opc_da_bindings/src/server/mod.rs`:
- Around line 531-541: Update the write handling around value_from_abi and the
current collect::<Result<_>>()? so conversion is performed independently for
each item. Record the appropriate per-item failure HRESULT in errors for values
that cannot be decoded, pass only successfully decoded (ServerItemHandle, value)
pairs to self.service.write, and preserve one result entry per input item
without aborting the entire Write call.
- Around line 177-182: In the successful interface-transfer path around
initialize_output, replace the unsafe transmute_copy(&output) pointer extraction
with the documented OutRef::write(None) API, provided it initializes the
out-parameter without consuming output. Remove the manual pointer cast and
retain the existing initialize_output error propagation and subsequent use of
output.
In `@opc_hda_bindings/src/client/mod.rs`:
- Around line 172-187: Update the output-array handling in the client method
around the unsafe set_len and into_array calls so the server-provided count is
used as the cleanup length only when call succeeds. When call.is_err(), keep the
array lengths at zero before transferring ownership, while preserving the
existing successful-call conversion and error propagation for ids, names,
descriptions, and data_types.
In `@opc_hda_bindings/src/lib.rs`:
- Around line 3-7: Document the breaking API change associated with the
crate-level `pub(crate) use bindings::*` in the README or changelog: generated
Windows ABI symbols such as `IOPCHDA_Server`, `IOPCHDA_SyncRead`, and
`tagOPCHDA_ITEM` are no longer publicly exported. Update the existing adapter
documentation to reflect their removal while preserving the current private
visibility.
In `@opc_hda_bindings/src/server/mod.rs`:
- Around line 337-339: Initialize all out-parameters before returning E_NOTIMPL
in CreateBrowse, ReadProcessed, ReadAtTime, ReadModified, and ReadAttribute,
using the existing initialize_output pattern and each method’s corresponding
output arguments (_browser, _errors, etc.).
- Around line 94-111: Update HdaServerAdapter to advertise IOPCCommon alongside
IOPCHDA_Server and IOPCHDA_SyncRead, and implement the required IOPCCommon
methods on the adapter. Ensure HdaServer::new continues creating its COM object
from the adapter so HdaClient::common() can successfully query the exposed
interface.
---
Outside diff comments:
In `@opc_da_bindings/src/client/mod.rs`:
- Around line 117-127: Update the `object.ok_or_else` handling in the `AddGroup`
flow to call `RemoveGroup` with `server_handle` and forced removal before
returning the unexpected-error result when no group object is returned. Preserve
the existing successful `DaGroup::new` path and propagate the original
missing-object error.
In `@opc_da_bindings/src/server/mod.rs`:
- Around line 213-231: The AddGroup flow currently inserts the GroupEntry before
output writes can fail, leaving an orphaned group on error. Move the self.groups
insertion until after output.write, server_handle.write, and
revised_update_rate.write complete successfully, preserving the existing error
propagation and group data.
---
Nitpick comments:
In `@opc_ae_bindings/src/abi.rs`:
- Around line 32-34: Update to_abi_error to construct the AbiError with the
HRESULT from error.code().raw() and the original error.message(), using
AbiError::new instead of AbiError::from_hresult, so service_call preserves
diagnostic text for in-process callers.
In `@opc_ae_bindings/src/client/mod.rs`:
- Around line 527-534: Update the event subscription construction around
CreateEventSubscription to use the returned IUnknown directly, removing the
object_from_interface conversion and subsequent interface_from_object
QueryInterface call. Preserve the null-response error handling and populate
EventSubscription.inner from the returned interface without introducing another
COM round trip.
In `@opc_ae_bindings/src/server/mod.rs`:
- Around line 441-459: The round-trip test
client_and_server_round_trip_without_public_windows_types must cover the
adapter’s rejection and error-propagation paths. Add assertions for
source_conditions, disable_areas, enable_sources, and disable_sources, verify
create_subscription and area_browser return the not_implemented error, and add a
service whose status() returns Err(...) to confirm the resulting error code
survives ABI conversion.
In `@opc_classic_types/src/error.rs`:
- Around line 152-161: Extend the #[cfg(test)] coverage in the foundational
types: add assertions for Error::from_code kind mapping, verify
Timestamp::from_system_time and to_system_time round-trip consistently, and
create a com.rs test module covering the Guid Debug format. Keep the existing
failure/success status-bit test unchanged.
In `@opc_classic_types/src/value.rs`:
- Around line 62-90: Extend the From implementations for Value to cover u32,
i16, u16, i64, u64, f32, and Vec<u8>, mapping each input to its corresponding
Value variant. Use a declarative macro to generate the scalar and existing
conversions consistently, while preserving the current String and &str ownership
behavior.
In `@opc_classic_utils/src/memory/array.rs`:
- Around line 255-321: Extend the existing tests for CoTaskMemArrayBuilder with
two transactional cases: verify that finishing via into_raw_parts does not clean
up initialized elements, and verify that pushing into a full builder returns
Err(value) with the original value still owned by the caller. Reuse the per-test
Arc<AtomicUsize> drop counter and existing CountDrop/DropElements helpers, and
assert the expected cleanup counts and returned value.
In `@opc_classic_utils/src/memory/out.rs`:
- Around line 165-179: Add a pure-Rust test alongside
array_output_deep_cleans_when_not_adopted that constructs a CoTaskMemArrayOut
with tracked CountDrop elements, writes the allocated pointer, calls set_len,
and transfers ownership through into_array. Drop the resulting array and assert
the drop counter reports exactly one cleanup per element, covering the adopted
transfer path and set_len behavior.
In `@opc_classic_utils/src/server.rs`:
- Around line 163-189: Update register_in_context to accept a configurable
REGCLS value alongside ClassContext and pass it to CoRegisterClassObject,
enabling single-use or suspended registrations. If the API must remain fixed to
REGCLS_MULTIPLEUSE, add a doc comment to register_in_context explicitly
documenting that limitation.
In `@opc_comn_bindings/src/abi.rs`:
- Around line 11-13: Update to_abi_error to construct the AbiError with
AbiError::new, passing the existing HRESULT derived from error.code().raw() and
error.message() so both the error code and original message are preserved.
In `@opc_comn_bindings/src/server/mod.rs`:
- Around line 419-437: Remove the argument assertions from the service methods
enum_classes, class_details, and class_id_from_prog_id; instead, record each
received argument in the existing shared test state and assert the expected
values in the test body after the ABI call completes, preserving the methods’
current return values.
In `@opc_da_bindings/src/server/mod.rs`:
- Around line 820-827: Update next_nonzero to handle AtomicU32 wraparound
safely: prevent returning a value already present in the live groups map, or
permanently fail once wraparound is detected. Preserve the existing rejection of
invalid counter values and ensure generated handles remain unique among active
groups.
In `@opc_hda_bindings/src/client/mod.rs`:
- Around line 360-365: Update HdaClient to cache the IOPCHDA_SyncRead interface
during construction or lazy initialization, then reuse that cached interface in
read_raw instead of cloning self.object() and calling interface_from_object on
every invocation. Preserve the existing read behavior and error propagation.
- Around line 531-533: The u32 count conversion is duplicated between the client
and server modules. In opc_hda_bindings/src/client/mod.rs lines 531-533, retain
or move count into crate::convert as the shared helper; in
opc_hda_bindings/src/server/mod.rs lines 597-599, remove checked_count and
update its callers to use the shared count helper, preserving the existing error
behavior.
- Around line 523-529: Update pwstr_string to stop silently converting invalid
UTF-16 to an empty String: preserve null-pointer handling, but propagate the
conversion failure as the crate’s established invalid-argument error (matching
opc_da_bindings), or use a lossy conversion that retains recoverable text
instead of defaulting to empty.
In `@opc_hda_bindings/src/convert.rs`:
- Around line 45-122: Move value_to_abi, value_from_abi, scalar_variant, and
their shared tests from the binding-specific conversion modules into
opc_classic_utils, preserving the existing Automation mappings and error
behavior. Update the DA and HDA modules to re-export the shared helpers instead
of defining local copies, and ensure AE uses the same shared conversion layer so
all binding crates rely on one implementation.
In `@opc_hda_bindings/src/server/mod.rs`:
- Around line 378-390: Handle failures from build_hda_item within the per-item
loop instead of propagating them with ?. Update the Ok(item) arm to convert
conversion errors into the corresponding item HRESULT, append the default
tagOPCHDA_ITEM value, and set has_error, matching the existing Err(error)
behavior while allowing the remaining samples to be processed.
- Around line 724-755: The round-trip test
client_and_server_adapters_round_trip_nested_task_memory currently covers only
successful operations; extend it to exercise failure mappings and cleanup. Add
assertions for HdaTime::Expression, an item producing
HdaServerItemResult::failure, and validate_item_ids, verifying the expected
partial-success/error results and nested tagOPCHDA_ITEM cleanup while preserving
the existing success assertions.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 43371e6e-f5cc-4d72-a831-aece3039886a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (46)
Cargo.tomlREADME.mdopc_ae_bindings/Cargo.tomlopc_ae_bindings/README.mdopc_ae_bindings/src/abi.rsopc_ae_bindings/src/client/mod.rsopc_ae_bindings/src/lib.rsopc_ae_bindings/src/server/mod.rsopc_ae_bindings/src/types.rsopc_classic_types/Cargo.tomlopc_classic_types/README.mdopc_classic_types/src/com.rsopc_classic_types/src/error.rsopc_classic_types/src/lib.rsopc_classic_types/src/value.rsopc_classic_utils/Cargo.tomlopc_classic_utils/README.mdopc_classic_utils/examples/ownership.rsopc_classic_utils/src/com.rsopc_classic_utils/src/lib.rsopc_classic_utils/src/memory/array.rsopc_classic_utils/src/memory/mod.rsopc_classic_utils/src/memory/out.rsopc_classic_utils/src/memory/wide.rsopc_classic_utils/src/server.rsopc_comn_bindings/Cargo.tomlopc_comn_bindings/README.mdopc_comn_bindings/src/abi.rsopc_comn_bindings/src/client/mod.rsopc_comn_bindings/src/lib.rsopc_comn_bindings/src/server/mod.rsopc_da_bindings/Cargo.tomlopc_da_bindings/README.mdopc_da_bindings/src/abi.rsopc_da_bindings/src/client/group.rsopc_da_bindings/src/client/mod.rsopc_da_bindings/src/client/properties.rsopc_da_bindings/src/client/types.rsopc_da_bindings/src/lib.rsopc_da_bindings/src/server/mod.rsopc_hda_bindings/Cargo.tomlopc_hda_bindings/README.mdopc_hda_bindings/src/client/mod.rsopc_hda_bindings/src/convert.rsopc_hda_bindings/src/lib.rsopc_hda_bindings/src/server/mod.rs
🚧 Files skipped from review as they are similar to previous changes (14)
- opc_ae_bindings/README.md
- opc_comn_bindings/Cargo.toml
- README.md
- opc_comn_bindings/README.md
- opc_da_bindings/Cargo.toml
- opc_classic_utils/src/memory/mod.rs
- opc_classic_utils/src/lib.rs
- opc_classic_utils/src/com.rs
- opc_classic_utils/README.md
- opc_ae_bindings/Cargo.toml
- opc_classic_utils/src/memory/wide.rs
- opc_hda_bindings/Cargo.toml
- opc_da_bindings/src/client/group.rs
- Cargo.toml
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
opc_classic_abi/src/lib.rs (2)
89-122: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake out-pointer handling consistent across the thunks.
Two issues in this vtable:
available_localesforwardscountandlocalesto the impl without a null check, whileget_localeanderror_stringreturnE_POINTERfor a null out pointer. The in-tree impls do validate (opc_da_bindings/src/server/mod.rslines 173-195 callinitialize_output), so there is no current defect. ButIOPCCommon_Implis public, so the safety requirement now depends on every implementer remembering the check. Either null-check in the thunk or document the requirement on the trait method.
get_localeanderror_stringwrite the out parameter only on the success path. If the impl returnsErr, the caller buffer keeps its previous value. COM callers commonly initialize out parameters themselves, but generatedwindows-bindgenthunks zero the output before returning the error. Zeroing here removes the chance that a caller which ignores the HRESULT reads a stale locale or a stalePWSTRand then frees it.🛡️ Proposed fix for the error-path output
unsafe { let this = &*((this as *const *const ()).offset(OFFSET) as *const Identity); match IOPCCommon_Impl::GetLocaleID(this) { Ok(value) => { locale.write(value); windows_core::HRESULT(0) } - Err(error) => error.into(),+ Err(error) => {+ locale.write(0);+ error.into()+ } } }unsafe { let this = &*((this as *const *const ()).offset(OFFSET) as *const Identity); match IOPCCommon_Impl::GetErrorString(this, error) { Ok(value) => { output.write(value); windows_core::HRESULT(0) } - Err(error) => error.into(),+ Err(error) => {+ output.write(windows_core::PWSTR::null());+ error.into()+ } } }🤖 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 `@opc_classic_abi/src/lib.rs` around lines 89 - 122, Make the IOPCCommon vtable thunks enforce consistent out-pointer handling: update available_locales to return E_POINTER when count or locales is null, and initialize the output in get_locale and error_string before invoking their implementations so failed calls leave a null/zero value. Preserve existing success writes and HRESULT propagation.
10-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid hand-writing
IOPCCommonwithwindows_core::imp.
windows_core::impis an internal/private Windows-rs module, sodefine_interface!andinterface_hierarchy!can break or change without semver protection. GenerateIOPCCommonwithwindows-bindgenfrom the OPC COM IDL, then use the generated binding instead.🤖 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 `@opc_classic_abi/src/lib.rs` around lines 10 - 15, Replace the hand-written IOPCCommon declaration in the lib.rs module with the generated windows-bindgen binding from the OPC COM IDL, since windows_core::imp::define_interface! and interface_hierarchy! are internal APIs. Update the IOPCCommon symbol to come from the generated binding and remove the direct windows_core::imp usage from this interface definition while preserving the same COM identity and hierarchy behavior through the generated code.opc_classic_utils/src/memory/out.rs (1)
76-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the
set_lencompatibility alias.
CoTaskMemArrayOutdoes not have earlier released callers in this workspace. The only in-repo caller is the test that exercises the alias, so keeping a second publicunsafename only duplicates the trust transition. Removeset_lenand update the test to callcommit_len.🤖 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 `@opc_classic_utils/src/memory/out.rs` around lines 76 - 85, Remove the public unsafe CoTaskMemArrayOut::set_len compatibility alias, and update the test that exercises it to call commit_len instead. Preserve commit_len as the sole API for committing the length and remove any alias-specific documentation or references.
🤖 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 `@opc_da_bindings/src/server/mod.rs`:
- Around line 240-248: Update the output handling in the AddGroup path so
initialization nulls the underlying interface pointer rather than the local
OutRef wrapper. Replace the transmute-based initialize_output call with a
supported OutRef pattern that preserves null-on-failure behavior and leaves
output valid for its later write(...) transfer. Ensure successful AddGroup calls
return the created group interface instead of E_POINTER.
- Around line 280-298: Update the AddGroup flow to allocate server_handle before
calling DaService::add_group, then pass that generated handle into add_group and
reuse it when writing the output and inserting the GroupEntry. Ensure
DaService::remove_group receives the same handle associated with the created
group, eliminating the later handle generation.
- Around line 603-622: Update the Write implementation around owned,
conversion_errors, and merged to use Vec::new() followed by
try_reserve_exact(count as usize), propagating allocation failure as
E_OUTOFMEMORY. Reserve owned and conversion_errors before the input loop, and
reserve merged before calling self.service.write(&owned)?; remove the existing
with_capacity allocations while preserving the current write and merge behavior.
---
Nitpick comments:
In `@opc_classic_abi/src/lib.rs`:
- Around line 89-122: Make the IOPCCommon vtable thunks enforce consistent
out-pointer handling: update available_locales to return E_POINTER when count or
locales is null, and initialize the output in get_locale and error_string before
invoking their implementations so failed calls leave a null/zero value. Preserve
existing success writes and HRESULT propagation.
- Around line 10-15: Replace the hand-written IOPCCommon declaration in the
lib.rs module with the generated windows-bindgen binding from the OPC COM IDL,
since windows_core::imp::define_interface! and interface_hierarchy! are internal
APIs. Update the IOPCCommon symbol to come from the generated binding and remove
the direct windows_core::imp usage from this interface definition while
preserving the same COM identity and hierarchy behavior through the generated
code.
In `@opc_classic_utils/src/memory/out.rs`:
- Around line 76-85: Remove the public unsafe CoTaskMemArrayOut::set_len
compatibility alias, and update the test that exercises it to call commit_len
instead. Preserve commit_len as the sole API for committing the length and
remove any alias-specific documentation or references.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ab16bc18-9803-40b9-9d33-6b05365e1c2e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
Cargo.tomlREADME.mdopc_ae_bindings/Cargo.tomlopc_ae_bindings/README.mdopc_ae_bindings/src/abi.rsopc_ae_bindings/src/client/mod.rsopc_ae_bindings/src/server/mod.rsopc_classic_abi/Cargo.tomlopc_classic_abi/src/lib.rsopc_classic_types/src/com.rsopc_classic_types/src/error.rsopc_classic_types/src/value.rsopc_classic_utils/README.mdopc_classic_utils/src/memory/array.rsopc_classic_utils/src/memory/mod.rsopc_classic_utils/src/memory/object.rsopc_classic_utils/src/memory/out.rsopc_classic_utils/src/server.rsopc_comn_bindings/src/abi.rsopc_comn_bindings/src/client/mod.rsopc_comn_bindings/src/server/mod.rsopc_da/.gitignoreopc_da_bindings/Cargo.tomlopc_da_bindings/README.mdopc_da_bindings/src/abi.rsopc_da_bindings/src/client/mod.rsopc_da_bindings/src/client/properties.rsopc_da_bindings/src/server/mod.rsopc_hda_bindings/Cargo.tomlopc_hda_bindings/README.mdopc_hda_bindings/src/client/mod.rsopc_hda_bindings/src/convert.rsopc_hda_bindings/src/server/mod.rs
💤 Files with no reviewable changes (1)
- opc_da/.gitignore
🚧 Files skipped from review as they are similar to previous changes (19)
- opc_hda_bindings/Cargo.toml
- opc_da_bindings/README.md
- opc_comn_bindings/src/abi.rs
- opc_classic_types/src/com.rs
- opc_ae_bindings/Cargo.toml
- opc_classic_utils/README.md
- opc_ae_bindings/src/client/mod.rs
- opc_hda_bindings/src/client/mod.rs
- Cargo.toml
- opc_comn_bindings/src/client/mod.rs
- opc_da_bindings/src/client/properties.rs
- opc_da_bindings/Cargo.toml
- opc_da_bindings/src/abi.rs
- opc_hda_bindings/src/convert.rs
- opc_da_bindings/src/client/mod.rs
- opc_classic_utils/src/memory/array.rs
- opc_classic_utils/src/server.rs
- opc_comn_bindings/src/server/mod.rs
- opc_ae_bindings/src/server/mod.rs
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Superseded by the reviewed fixes on HEAD 753ffa0; latest CodeRabbit run reported no actionable comments.
Uh oh!
There was an error while loading. Please reload this page.
Summary by CodeRabbit