Fix shadowing in ompi #8593
Fix shadowing in ompi #8593
Conversation
|
Opened as a result of discussion in #8591 More changes to come. |
|
The IBM CI (GNU/Scale) build failed! Please review the log, linked below. Gist: https://gist.github.com/368de65d3cf2ca397306a2802dc8afda |
|
The IBM CI (XL) build failed! Please review the log, linked below. Gist: https://gist.github.com/84a9668b9a0dc6fa9daa31447e82982e |
|
The IBM CI (PGI) build failed! Please review the log, linked below. Gist: https://gist.github.com/925db533557028ed50040dfc0ae20c02 |
|
This does not improve the readability of the code. Plus in all instances addressed in this patch the scoping was correct, so there was nothing to fix. |
|
Thanks for the comments. There's more to be added before it's ready to be merged. In general shadowing is a bad practice that can lead to unintended bugs, so I'd argue that it is a 'good' thing. But if the consensus is we don't want it I won't pursue it further. |
|
Yeah. I think this is a good thing. I have in the past encountered issues where shadowing covered up a real bug in Open MPI. I think this is a good thing to get a consensus decision on. |
|
We've been burned multiple times this way - let's stop shadowing. |
| @@ -382,7 +382,6 @@ int mca_topo_treematch_dist_graph_create(mca_topo_base_module_t* topo_module, | |||
|
|
|||
| /* Centralized Reordering */ | |||
| if (0 == mca_topo_treematch_component.reorder_mode) { | |||
| int *k = NULL; | |||
There was a problem hiding this comment.
I'm a little unsure of this change. I think this is equivalent, but I'm not sure what k is supposed to be, and this function is huge.
|
Looks like this is already uncovering some bugs. @awlauria If you find a bug due to shadowing please open separate tickets for those issues and PR to v4.0.x, v4.1.x, and v5.0.x. |
awlauria
force-pushed
the
fix_shadowing
branch
3 times, most recently
from
5871531 to
8827c18
Compare
| @@ -445,39 +443,35 @@ mca_btl_base_rdma_start (mca_btl_base_module_t *btl, struct mca_btl_base_endpoin | |||
| context->local_handle = local_handle; | |||
| context->total_size = size; | |||
|
|
|||
| size_t send_size = 0; | |||
| size_t recv_size = 0; | |||
There was a problem hiding this comment.
@hjelmn This was marked as unused, was this intended to get added to the packet_size or should this block be removed/refactored to not use it:
else if (MCA_BTL_BASE_AM_GET == type) {
if (sizeof (mca_btl_base_rdma_response_hdr_t) + size <= btl->btl_eager_limit) {
recv_size = size;
packet_size += size;
} else if (!mca_btl_base_rdma_use_rdma_put (btl)) {
recv_size = size_t_min (size, btl->btl_max_send_size -
packet_size += size_t_min (size, btl->btl_max_send_size -
sizeof (mca_btl_base_rdma_response_hdr_t));
} else {
use_rdma = true;
}
} else {
/* fetching atomic. always recv result via active message */
recv_size = size;
packet_size += size;
}
There was a problem hiding this comment.
@hjelmn when you get a chance can you look at the above?
|
Removing the WIP label. All of the shadowing caught with gcc should be removed. There's some in treematch and romio that I didn't touch, and likely never will. I also added a commit that addressed some low hanging misc. warnings. |
|
Trying again... bot:aws:retest |
awlauria
force-pushed
the
fix_shadowing
branch
2 times, most recently
from
9f435a2 to
04dff2b
Compare
|
bot:ompi:retest |
Signed-off-by: Austen Lauria <[email protected]>
Signed-off-by: Austen Lauria <[email protected]>
Signed-off-by: Austen Lauria <[email protected]>
|
Rebased on top of master. |
|
bot:aws:retest |
No description provided.