Codechange: make explicit when a TileIndex is cast to its basetype #11190
Conversation
TrueBrain
force-pushed
the
strong-type-tileindex
branch
3 times, most recently
from
e6f74f1
to
d71df9f
Compare
TrueBrain
force-pushed
the
strong-type-tileindex
branch
from
d71df9f
to
fcba839
Compare
TrueBrain
force-pushed
the
strong-type-tileindex
branch
2 times, most recently
from
23e20ba
to
c5e6521
Compare
| @@ -151,7 +151,7 @@ Industry::~Industry() | |||
| for (TileIndex tile_cur : this->location) { | |||
| if (IsTileType(tile_cur, MP_INDUSTRY)) { | |||
| if (GetIndustryIndex(tile_cur) == this->index) { | |||
| DeleteNewGRFInspectWindow(GSF_INDUSTRYTILES, tile_cur); | |||
| DeleteNewGRFInspectWindow(GSF_INDUSTRYTILES, static_cast<uint32_t>(tile_cur)); | |||
There was a problem hiding this comment.
This function shows up a lot in this PR. Could it be templated to accept a TileIndex without casting each time it's called?
There was a problem hiding this comment.
It is a balance. I didn't want to silently case the strong-typed values for the newgrf inspect window, as honestly, that felt a bit wrong in these cases. It depends on the enum what the type of the second thing is, and that deserves its own proper solution. But not for this PR.
Similar can be argued for the window-related functions, but there were just too many of them to do it like that.
So yeah, meh. I don't want to sparkles templated functions everywhere; for SetDParam and window_number I can see the argument. Here .. I was much more hesitant. So that is why I went for this approach. I rather not add templates functions to just cast away the strong typing when possible. "Work by exception" approach :)
Let me know if you disagree. It is an opinion, not a black/white thing :D
There was a problem hiding this comment.
I don't know if I'm qualified to have an opinion, but looking through code with casts everywhere can be confusing. And we know the timeline of "we'll give it a proper solution in another PR someday." 😛
I wonder if a function overload would clean up the function calls to cast in one central place, or if that's just half-assing the eventual correct solution.
In any case, none of it bothers me enough to prevent an approval. If you decide to change it, I'll re-approve.
TrueBrain
force-pushed
the
strong-type-tileindex
branch
from
c5e6521
to
53ab025
Compare
This prevents people accidentially assigning a TileIndex to a Date or any other type they shouldn't.
TrueBrain
force-pushed
the
strong-type-tileindex
branch
from
53ab025
to
dd2b2fd
Compare
This PR includes #10761 as it depends on it.
Motivation / Problem
StrongTypeDef allowed, before #10761 already, implicit conversion to
uint32_tforTileIndex, because of the many many many cases this was needed. But because of #10761, the amount of places this is still happening is reduced by a lot. This is mostly because functions likeSetDParamnow accept aStrongTypeDef, and knows how to deal with it.This made me wonder: can we make
TileIndex's conversion explicit, so we don't accidentally assign incompatible types to each other just because the compiler used an implicit conversion.Description
Turns out, the answer is: yes, we can. And this PR is the result of that.
dest_tileis misused by disasters to store an ID. Not part of this PR to ... "fix" that issue.The
TileIndexDiffis a bit weird; that is not normally how we calculate aTileIndexDiff. So changed it to what others do too.Limitations
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.