From a9462eb0715a298c6813330e8e40f1971881e6df Mon Sep 17 00:00:00 2001 From: cgettys-microsoft <67080058+cgettys-microsoft@users.noreply.github.com> Date: Mon, 13 Jan 2025 15:28:37 -0800 Subject: [PATCH] Replace from_com and into_com with From implementations (#126) Given that these from_com and into_com methods are all safe anyway, there's no good reason not to use From instead Rewrite them as ```impl From for TYPE``` and ```impl From for TYPE```. In a few cases, the second (which would have been into_com before) did not exist, but made sense, so I added them. As per general Rust guidance, implementing From, as that also results in the blanket ```Into``` implementation. In the vast majority of cases, ownership was required, so I went with ```impl From for TYPE``` instead of ```impl From<&ITYPE> for TYPE```, as in some cases, this will save a reference count increment, and in the other case, it just moves where .clone is called, and those implementations won't be called frequently outside the library. This also saved a few unnecessary clones inside the library that we appear to have had by accident. Note in a few cases this makes it possible to construct the individual client wrappers outside the library (where we didn't have pub on from_com before). This is in my view a feature. This will require any code outside this library to replace from_com with from and into_com with .into, but that shouldn't be a big deal. I left ```FabricClient::from_com``` alone because the comment says the method is intended to be private eventually. --- crates/libs/core/src/client/connection.rs | 8 +++--- crates/libs/core/src/client/health_client.rs | 16 +++++++---- crates/libs/core/src/client/mod.rs | 21 +++++++------- crates/libs/core/src/client/notification.rs | 22 +++++++++++---- crates/libs/core/src/client/query_client.rs | 14 ++++++---- .../libs/core/src/client/svc_mgmt_client.rs | 28 ++++++++++++------- .../core/src/runtime/activation_context.rs | 4 +-- crates/libs/core/src/runtime/config.rs | 12 ++++++-- .../core/src/runtime/package_change/config.rs | 11 ++++---- crates/libs/core/src/types/client/node.rs | 8 ++++-- 10 files changed, 90 insertions(+), 54 deletions(-) diff --git a/crates/libs/core/src/client/connection.rs b/crates/libs/core/src/client/connection.rs index 6e43478..27b92a6 100644 --- a/crates/libs/core/src/client/connection.rs +++ b/crates/libs/core/src/client/connection.rs @@ -27,8 +27,8 @@ pub struct GatewayInformationResult { pub node_name: crate::WString, } -impl GatewayInformationResult { - fn from_com(com: &IFabricGatewayInformationResult) -> Self { +impl From<&IFabricGatewayInformationResult> for GatewayInformationResult { + fn from(com: &IFabricGatewayInformationResult) -> Self { let info = unsafe { com.get_GatewayInformation().as_ref().unwrap() }; Self { node_address: WStringWrap::from(info.NodeAddress).into(), @@ -69,7 +69,7 @@ where &self, gw_info: windows_core::Ref, ) -> windows_core::Result<()> { - let info = GatewayInformationResult::from_com(gw_info.unwrap()); + let info = GatewayInformationResult::from(gw_info.unwrap()); self.inner.on_connected(&info) } @@ -77,7 +77,7 @@ where &self, gw_info: windows_core::Ref, ) -> windows_core::Result<()> { - let info = GatewayInformationResult::from_com(gw_info.unwrap()); + let info = GatewayInformationResult::from(gw_info.unwrap()); self.inner.on_disconnected(&info) } } diff --git a/crates/libs/core/src/client/health_client.rs b/crates/libs/core/src/client/health_client.rs index 919a091..25d00dd 100644 --- a/crates/libs/core/src/client/health_client.rs +++ b/crates/libs/core/src/client/health_client.rs @@ -31,16 +31,20 @@ pub struct HealthClient { com: IFabricHealthClient4, } -// Public implementation block -impl HealthClient { - pub fn get_com(&self) -> IFabricHealthClient4 { - self.com.clone() +impl From for HealthClient { + fn from(value: IFabricHealthClient4) -> Self { + Self { com: value } } +} - pub fn from_com(com: IFabricHealthClient4) -> Self { - Self { com: com.clone() } +impl From for IFabricHealthClient4 { + fn from(value: HealthClient) -> Self { + value.com } +} +// Public implementation block +impl HealthClient { /// Reports health on a Service Fabric entity. See C# API [here](https://docs.microsoft.com/en-us/dotnet/api/system.fabric.fabricclient.healthclient.reporthealth?view=azure-dotnet). /// /// Remarks: diff --git a/crates/libs/core/src/client/mod.rs b/crates/libs/core/src/client/mod.rs index a74f9b8..d8960eb 100644 --- a/crates/libs/core/src/client/mod.rs +++ b/crates/libs/core/src/client/mod.rs @@ -226,10 +226,10 @@ impl FabricClient { let com_query_client = com.clone().cast::().unwrap(); let com_health_client = com.clone().cast::().unwrap(); Self { - property_client: PropertyManagementClient::from_com(com_property_client), - service_client: ServiceManagementClient::from_com(com_service_client), - query_client: QueryClient::from_com(com_query_client), - health_client: HealthClient::from_com(com_health_client), + property_client: PropertyManagementClient::from(com_property_client), + service_client: ServiceManagementClient::from(com_service_client), + query_client: QueryClient::from(com_query_client), + health_client: HealthClient::from(com_health_client), } } @@ -259,13 +259,14 @@ pub struct PropertyManagementClient { com: IFabricPropertyManagementClient2, } -impl PropertyManagementClient { - /// Get a copy of COM object - pub fn get_com(&self) -> IFabricPropertyManagementClient2 { - self.com.clone() +impl From for PropertyManagementClient { + fn from(com: IFabricPropertyManagementClient2) -> Self { + Self { com } } +} - fn from_com(com: IFabricPropertyManagementClient2) -> Self { - Self { com } +impl From for IFabricPropertyManagementClient2 { + fn from(value: PropertyManagementClient) -> Self { + value.com } } diff --git a/crates/libs/core/src/client/notification.rs b/crates/libs/core/src/client/notification.rs index 3a9c226..ced6a63 100644 --- a/crates/libs/core/src/client/notification.rs +++ b/crates/libs/core/src/client/notification.rs @@ -37,8 +37,8 @@ pub struct ServiceNotification { com: IFabricServiceNotification, } -impl ServiceNotification { - fn from_com(com: IFabricServiceNotification) -> Self { +impl From for ServiceNotification { + fn from(com: IFabricServiceNotification) -> Self { // SF guarantees this is not null. let raw = unsafe { com.get_Notification().as_ref().unwrap() }; Self { @@ -56,10 +56,12 @@ impl ServiceNotification { com, } } +} +impl ServiceNotification { pub fn get_version(&self) -> crate::Result { let version = unsafe { self.com.GetVersion() }?; - Ok(ServiceEndpointsVersion::from_com(version)) + Ok(ServiceEndpointsVersion::from(version)) } } @@ -96,11 +98,19 @@ pub struct ServiceEndpointsVersion { com: IFabricServiceEndpointsVersion, } -impl ServiceEndpointsVersion { - fn from_com(com: IFabricServiceEndpointsVersion) -> Self { +impl From for ServiceEndpointsVersion { + fn from(com: IFabricServiceEndpointsVersion) -> Self { Self { com } } +} +impl From for IFabricServiceEndpointsVersion { + fn from(value: ServiceEndpointsVersion) -> Self { + value.com + } +} + +impl ServiceEndpointsVersion { /// CSharp doc: Zero if this and other are equivalent, /// a negative value if this is less than other, and a positive value if this is greater than other. /// @@ -167,7 +177,7 @@ where notification: windows_core::Ref, ) -> crate::Result<()> { let com = notification.unwrap(); - let msg = ServiceNotification::from_com(com.to_owned()); + let msg = ServiceNotification::from(com.clone()); self.inner.on_notification(&msg) } } diff --git a/crates/libs/core/src/client/query_client.rs b/crates/libs/core/src/client/query_client.rs index 680f9e8..6b2e564 100644 --- a/crates/libs/core/src/client/query_client.rs +++ b/crates/libs/core/src/client/query_client.rs @@ -113,13 +113,15 @@ impl QueryClient { } } -impl QueryClient { - pub fn get_com(&self) -> IFabricQueryClient10 { - self.com.clone() +impl From for QueryClient { + fn from(com: IFabricQueryClient10) -> Self { + Self { com } } +} - pub fn from_com(com: IFabricQueryClient10) -> Self { - Self { com: com.clone() } +impl From for IFabricQueryClient10 { + fn from(value: QueryClient) -> Self { + value.com } } @@ -161,7 +163,7 @@ impl QueryClient { ) } .await??; - Ok(NodeList::from_com(com)) + Ok(NodeList::from(com)) } pub async fn get_partition_list( diff --git a/crates/libs/core/src/client/svc_mgmt_client.rs b/crates/libs/core/src/client/svc_mgmt_client.rs index 85c8c41..b5e9b16 100644 --- a/crates/libs/core/src/client/svc_mgmt_client.rs +++ b/crates/libs/core/src/client/svc_mgmt_client.rs @@ -150,10 +150,15 @@ impl ServiceManagementClient { } } -// public implementation block -impl ServiceManagementClient { - pub fn from_com(com: IFabricServiceManagementClient6) -> Self { - Self { com: com.clone() } +impl From for ServiceManagementClient { + fn from(com: IFabricServiceManagementClient6) -> Self { + Self { com } + } +} + +impl From for IFabricServiceManagementClient6 { + fn from(value: ServiceManagementClient) -> Self { + value.com } } @@ -186,7 +191,7 @@ impl ServiceManagementClient { ) } .await??; - let res = ResolvedServicePartition::from_com(com); + let res = ResolvedServicePartition::from(com); Ok(res) } @@ -377,8 +382,8 @@ pub struct ResolvedServicePartition { com: IFabricResolvedServicePartitionResult, } -impl ResolvedServicePartition { - fn from_com(com: IFabricResolvedServicePartitionResult) -> Self { +impl From for ResolvedServicePartition { + fn from(com: IFabricResolvedServicePartitionResult) -> Self { Self { com } } } @@ -408,7 +413,7 @@ impl ResolvedServicePartition { // Get the list of endpoints pub fn get_endpoint_list(&self) -> ResolvedServiceEndpointList { - ResolvedServiceEndpointList::from_com(self.com.clone()) + ResolvedServiceEndpointList::from(self.com.clone()) } // If compared with different partition error is returned. @@ -472,10 +477,13 @@ pub struct ResolvedServiceEndpointList { com: IFabricResolvedServicePartitionResult, } -impl ResolvedServiceEndpointList { - fn from_com(com: IFabricResolvedServicePartitionResult) -> Self { +impl From for ResolvedServiceEndpointList { + fn from(com: IFabricResolvedServicePartitionResult) -> Self { Self { com } } +} + +impl ResolvedServiceEndpointList { // Get iterator for the list pub fn iter(&self) -> ResolvedServiceEndpointListIter { ResolvedServiceEndpointListIter::new(self, self) diff --git a/crates/libs/core/src/runtime/activation_context.rs b/crates/libs/core/src/runtime/activation_context.rs index 40afb8a..634ec9c 100644 --- a/crates/libs/core/src/runtime/activation_context.rs +++ b/crates/libs/core/src/runtime/activation_context.rs @@ -78,7 +78,7 @@ impl CodePackageActivationContext { self.com_impl .GetConfigurationPackage(configpackagename.as_pcwstr()) }?; - Ok(ConfigurationPackage::from_com(c)) + Ok(ConfigurationPackage::from(c)) } pub fn get_code_package_info(&self) -> CodePackageInfo { @@ -162,7 +162,7 @@ impl CodePackageActivationContext { .RegisterConfigurationPackageChangeHandler(&callback) }?; // SAFETY: raw_handle is a configuration package change handler id, not some other id. - Ok(unsafe { ConfigurationPackageChangeCallbackHandle::from_com(raw_handle) }) + Ok(unsafe { ConfigurationPackageChangeCallbackHandle::from(raw_handle) }) } pub fn unregister_configuration_package_change_handler( diff --git a/crates/libs/core/src/runtime/config.rs b/crates/libs/core/src/runtime/config.rs index f16304a..d1b6ece 100644 --- a/crates/libs/core/src/runtime/config.rs +++ b/crates/libs/core/src/runtime/config.rs @@ -62,11 +62,19 @@ impl FabricListAccessor for ConfigurationSectionLi } } -impl ConfigurationPackage { - pub fn from_com(com: IFabricConfigurationPackage) -> Self { +impl From for ConfigurationPackage { + fn from(com: IFabricConfigurationPackage) -> Self { Self { com } } +} + +impl From for IFabricConfigurationPackage { + fn from(value: ConfigurationPackage) -> Self { + value.com + } +} +impl ConfigurationPackage { pub fn get_description(&self) -> ConfigurationPackageDesc { let raw = unsafe { self.com.get_Description().as_ref().unwrap() }; diff --git a/crates/libs/core/src/runtime/package_change/config.rs b/crates/libs/core/src/runtime/package_change/config.rs index 66ef85e..390783f 100644 --- a/crates/libs/core/src/runtime/package_change/config.rs +++ b/crates/libs/core/src/runtime/package_change/config.rs @@ -47,7 +47,7 @@ where _source: windows_core::Ref, configpackage: windows_core::Ref, ) { - let new_package = ConfigurationPackage::from_com(configpackage.unwrap().clone()); + let new_package = ConfigurationPackage::from(configpackage.unwrap().clone()); let event = ConfigurationPackageChangeEvent::Addition { new_package }; self.inner.on_change(&event) } @@ -57,7 +57,7 @@ where _source: windows_core::Ref, configpackage: windows_core::Ref, ) { - let previous_package = ConfigurationPackage::from_com(configpackage.unwrap().clone()); + let previous_package = ConfigurationPackage::from(configpackage.unwrap().clone()); let event = ConfigurationPackageChangeEvent::Removal { previous_package }; self.inner.on_change(&event) } @@ -70,9 +70,8 @@ where >, configpackage: windows_core::Ref, ) { - let new_package = ConfigurationPackage::from_com(configpackage.unwrap().clone()); - let previous_package = - ConfigurationPackage::from_com(previousconfigpackage.unwrap().clone()); + let new_package = ConfigurationPackage::from(configpackage.unwrap().clone()); + let previous_package = ConfigurationPackage::from(previousconfigpackage.unwrap().clone()); let event = ConfigurationPackageChangeEvent::Modification { previous_package, new_package, @@ -117,7 +116,7 @@ pub struct ConfigurationPackageChangeCallbackHandle(pub(crate) i64); impl ConfigurationPackageChangeCallbackHandle { /// # Safety /// Caller ensures this is a registered callback id - pub const unsafe fn from_com(com: i64) -> Self { + pub const unsafe fn from(com: i64) -> Self { Self(com) } } diff --git a/crates/libs/core/src/types/client/node.rs b/crates/libs/core/src/types/client/node.rs index d92c8a1..b3c9f55 100644 --- a/crates/libs/core/src/types/client/node.rs +++ b/crates/libs/core/src/types/client/node.rs @@ -85,13 +85,17 @@ impl FabricListAccessor for NodeList { } } -impl NodeList { - pub fn from_com(com: IFabricGetNodeListResult2) -> Self { +impl From for NodeList { + fn from(com: IFabricGetNodeListResult2) -> Self { Self { com } } +} + +impl NodeList { pub fn iter(&self) -> NodeListIter { NodeListIter::new(self, self) } + pub fn get_paging_status(&self) -> Option { // If there is no more entries there is no paging status returned. let raw = unsafe { self.com.get_PagingStatus().as_ref() }?;