#12405: Refactor to use strong types #46
Conversation
patrickroberts
force-pushed
the
proberts/12405-strong-types
branch
from
360f6d4
to
20a8d0b
Compare
| namespace tt::umd { | ||
|
|
||
| enum class chip_id : int { | ||
| none = -1, | ||
| }; | ||
|
|
||
| enum class ethernet_channel : int {}; | ||
|
|
||
| struct eth_coord { | ||
| int x; | ||
| int y; | ||
| int rack; | ||
| int shelf; | ||
|
|
||
| // in C++20 this should be defined as: | ||
| // constexpr bool operator==(const eth_coord &other) const noexcept = default; | ||
| constexpr bool operator==(const eth_coord &other) const noexcept { | ||
| return (x == other.x and y == other.y and rack == other.rack and shelf == other.shelf); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Can establishing of a namespace for these structs come as another PR, where all code consistently use it from within that namespace and not global?
This pattern is widespread in the codebase today and I highly discourage it
There was a problem hiding this comment.
That's why the aliases are kept in place. I have not changed any existing usage of the aliases that were already there.
|
|
||
| using chip_id_t = tt::umd::chip_id; | ||
| using ethernet_channel_t = tt::umd::ethernet_channel; | ||
| using eth_coord_t = tt::umd::eth_coord; |
There was a problem hiding this comment.
if this is only merged to metal-main then you can potentially get rid of these *_t?
|
|
||
| namespace tt::umd { | ||
|
|
||
| enum class chip_id : int { |
There was a problem hiding this comment.
Having this as an enum can be a bit confusing, since you're using it such that any int value can be assigned to it, and it doesn't necessarily have a corresponding enum constant. However, I do see the value in using an explicit name for a type to have a clearer requirement and expectations over what is expected from an interface.
the eth_coord clearly benefits in readability from being defined as a struct, but whats the reasoning behind changing using chip_id_t to enum class?
You would then have define a constexpr int c_invalid_chip_id = -1;
There was a problem hiding this comment.
Ah actually, I saw the attached tt-metal issue, this ensures explicit conversion. And enum class is a different thing than enum :) LGTM
Ticket
tenstorrent/tt-metal#12405
Problem description
Aliases of integral types are being used interchangeably with the integral types, allowing for erroneous implicit conversions.
What's changed
Refactor to use enum class and make any intentional conversions explicit.