fix(tree): move declarations to top of function (aws#563)

c++ suppoorts initializers anywhere in the function, but one must not jump over an initializer with any goto usage. Given the lack of RAII in C, this becomes a significant painpoint. In large to-be-eventually-refactored functions contain gotos or use switch statements, split declaration and initialization, and move all declarations to the top of the function. This makes switch statements and gotos safe in both languages. Signed-off-by: Nicholas Sielicki <[email protected]>
aws-nslick · Sep 30, 2024 · 34c49d9 · 34c49d9
1 parent fb9216c
commit 34c49d9
Show file tree

Hide file tree

Showing 6 changed files with 109 additions and 70 deletions.
diff --git a/src/nccl_ofi_api.c b/src/nccl_ofi_api.c
@@ -365,15 +365,16 @@ ncclResult_t nccl_net_ofi_regMrDmaBuf(void* comm, void* data, size_t size,
 	const nccl_ofi_mr_ckey_t cache_key = nccl_ofi_mr_ckey_mk_vec(data, size);
 #endif
 
+	nccl_net_ofi_send_comm_t *send_comm = NULL;
+	nccl_net_ofi_recv_comm_t *recv_comm = NULL;
+
 	switch (base_comm->type) {
-	case NCCL_NET_OFI_SEND_COMM:;
-		nccl_net_ofi_send_comm_t *send_comm =
-			(nccl_net_ofi_send_comm_t *)base_comm;
+	case NCCL_NET_OFI_SEND_COMM:
+		send_comm = (nccl_net_ofi_send_comm_t *)base_comm;
 		ret = send_comm->regMr(send_comm, &cache_key, type, mhandle);
 		break;
-	case NCCL_NET_OFI_RECV_COMM:;
-		nccl_net_ofi_recv_comm_t *recv_comm =
-			(nccl_net_ofi_recv_comm_t *)base_comm;
+	case NCCL_NET_OFI_RECV_COMM:
+		recv_comm = (nccl_net_ofi_recv_comm_t *)base_comm;
 		ret = recv_comm->regMr(recv_comm, &cache_key, type, mhandle);
 		break;
 	default:
@@ -397,16 +398,16 @@ ncclResult_t nccl_net_ofi_deregMr(void *comm, void *mhandle)
 	}
 
 	int ret = 0;
+	nccl_net_ofi_send_comm_t *send_comm = NULL;
+	nccl_net_ofi_recv_comm_t *recv_comm = NULL;
 
 	switch (base_comm->type) {
-	case NCCL_NET_OFI_SEND_COMM:;
-		nccl_net_ofi_send_comm_t *send_comm =
-			(nccl_net_ofi_send_comm_t *)base_comm;
+	case NCCL_NET_OFI_SEND_COMM:
+		send_comm = (nccl_net_ofi_send_comm_t *)base_comm;
 		ret = send_comm->deregMr(send_comm, (nccl_net_ofi_mr_handle_t *)mhandle);
 		break;
-	case NCCL_NET_OFI_RECV_COMM:;
-		nccl_net_ofi_recv_comm_t *recv_comm =
-			(nccl_net_ofi_recv_comm_t *)base_comm;
+	case NCCL_NET_OFI_RECV_COMM:
+		recv_comm = (nccl_net_ofi_recv_comm_t *)base_comm;
 		ret = recv_comm->deregMr(recv_comm, (nccl_net_ofi_mr_handle_t *)mhandle);
 		break;
 	default:

diff --git a/src/nccl_ofi_ep_addr_list.c b/src/nccl_ofi_ep_addr_list.c
@@ -134,6 +134,7 @@ int nccl_ofi_ep_addr_list_insert(nccl_ofi_ep_addr_list_t *ep_list,
 				 size_t addr_size)
 {
 	int ret = 0;
+	ep_pair_list_elem_t *new_pair = NULL;
 
 	if (addr_size > ep_list->max_addr_size) {
 		NCCL_OFI_WARN("Address size %zu > max size (%zu)", addr_size,
@@ -155,8 +156,7 @@ int nccl_ofi_ep_addr_list_insert(nccl_ofi_ep_addr_list_t *ep_list,
 	memcpy(new_addr->addr, addr_in, addr_size);
 	zero_pad_address(new_addr->addr, addr_size, ep_list->max_addr_size);
 
-	ep_pair_list_elem_t *new_pair = (ep_pair_list_elem_t *)
-		malloc(sizeof(*new_pair));
+	new_pair = (ep_pair_list_elem_t *)malloc(sizeof(*new_pair));
 	if (!new_pair) {
 		NCCL_OFI_WARN("Failed to allocate new ep list element");
 		free(new_addr);

diff --git a/src/nccl_ofi_net.c b/src/nccl_ofi_net.c
@@ -136,6 +136,8 @@ int nccl_net_ofi_create_plugin(nccl_net_ofi_plugin_t **plugin_p)
 	int ret = 0;
 	const char *provider_filter = NULL;
 	nccl_net_ofi_plugin_t *plugin;
+	nccl_net_ofi_ep_t *base_ep = NULL;
+	nccl_net_ofi_device_t *device = NULL;
 
 	NCCL_OFI_INFO(NCCL_INIT | NCCL_NET, "Initializing " PACKAGE_STRING);
 
@@ -275,8 +277,7 @@ int nccl_net_ofi_create_plugin(nccl_net_ofi_plugin_t **plugin_p)
 	 * resources. This initialization happens once per process, and thus it
 	 * does not matter which device is used to create the endpoint.
 	 */
-	nccl_net_ofi_device_t *device = plugin->get_device(plugin, 0);
-	nccl_net_ofi_ep_t *base_ep = NULL;
+	device = plugin->get_device(plugin, 0);
 
 	ret = device->get_ep(device, &base_ep);
 	if (ret != 0) {