Do not pretend to default initialize a device lambda #5015
Conversation
Found in CCCL CI
bdice
added
improvement
| VertexValueInputIterator vertex_value_input_first{}; | ||
| VertexOp v_op{}; | ||
| vertex_t local_vertex_partition_range_first; | ||
| VertexValueInputIterator vertex_value_input_first; |
There was a problem hiding this comment.
Can't we default initialize iterators as well? Or is this because of possible thrust::transform_iterator with lambda?
There was a problem hiding this comment.
My reasoning with removing all default initializers is that partially initializing the struct does not make any sense and it is only ever initialized in a single place, so it feels "cleaner" to initialize all or nothing
Nothing I feel strongly about though
There was a problem hiding this comment.
OK, I am more inclined to default initializing variables that can be default initialized (it helps to prevent random non-deterministic behaviors due to garbage values), but I also don't feel strongly, either.
| vertex_t local_vertex_partition_range_first{}; | ||
| VertexValueInputIterator vertex_value_input_first{}; | ||
| VertexOp v_op{}; | ||
| vertex_t local_vertex_partition_range_first; |
There was a problem hiding this comment.
I think we can default initializing vertex_t (which is either int32_t or int64_t) or can this be also problematic?
There was a problem hiding this comment.
I believe we should be consistent, we only construct these types in one place
|
@miscco @seunghwak Checking status here. Can we land this in 25.06? |
| VertexValueInputIterator vertex_value_input_first{}; | ||
| VertexOp v_op{}; | ||
| vertex_t local_vertex_partition_range_first; | ||
| VertexValueInputIterator vertex_value_input_first; |
There was a problem hiding this comment.
OK, I am more inclined to default initializing variables that can be default initialized (it helps to prevent random non-deterministic behaviors due to garbage values), but I also don't feel strongly, either.
|
@bdice Sorry I just found that I've been neglecting my reviewer duty, just approved. |
|
/merge |
Found in CCCL CI