-
-
Notifications
You must be signed in to change notification settings - Fork 843
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Codechange: make TimerGameCalendar Date and Year types strongly typed #10761
Conversation
6a63b91
to
ad27508
Compare
I shifted gears, and went for a "I want to use The trick here is that to avoid 2 Sadly, I noticed that the compiler was sneaky converting everything back and forth between Anyway, much more interested in the overall idea of this PR, if we like defining such type this strongly. Does it actually help? |
ad27508
to
c512140
Compare
Another update. I decided to go for Mixins, after the idea of https://github.com/anthonywilliams/strong_typedef. What is nice about this, is that it means |
6d92021
to
6770f76
Compare
@TrueBrain A lot of this PR is way over my head with templating and casting, but you are absolutely right that we'll need this before #10700. Maybe some more experienced developers can weigh in on this? I would be eternally grateful. ❤️ |
6770f76
to
931505c
Compare
a893ddb
to
d517034
Compare
d517034
to
fc83675
Compare
The same way you make |
fc83675
to
e5d24e2
Compare
e5d24e2
to
2ccd421
Compare
2ccd421
to
f5a19e1
Compare
f5a19e1
to
d617b60
Compare
d617b60
to
5dc7513
Compare
5dc7513
to
97e2f9f
Compare
97e2f9f
to
20f2374
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand template stuff well enough to review it, but the casting all looks correct to me.
If someone else reviews the template stuff, I can continue working on #10700 tomorrow! Thanks again for taking this on! 😄
20f2374
to
a72775b
Compare
Motivation / Problem
With #10700 on the horizon, I am a bit afraid we will mix up Calendar with Economy, and get some weird buggy behaviour because of it. So I was wondering: can we make Date strongly typed, to avoid that issue.
This PR is a proof-of-concept for that. Curious what developers think.
And of course, by making it strongly typed, there is no actual functional change. It is just meant to assist the developer in not making silly mistakes. This was already fruitful, looking at all the PRs that came from this PR, with weird mistakes and boo-boos we made in our code-base.
Description
I went for a slightly different approach than
TileIndex
, as that requires defining a struct. I was more interested in usingusing
to create a new type. Got some inspiration from https://github.com/anthonywilliams/strong_typedef to make that happen.Initially, I made the conversion to
int32
implicit (so it auto-cases toint32
if it wants to). This had as nasty side-result that you could assign aYear
to aDate
and the other way around. As the compiler, as clever as it is, casted the one first to an int, to assign it to the other. This is kinda defeating the purpose of having strong-types to prevent people mixing up Calendar and Economy.So, making it explicit is the way to go. Sadly, that showed tons of places in our code where we take some ... freedom in terms of using a date/year to use as integer. The weirdest things. A lot of small PRs came from this to address most of these.
Limitations
Although performance shouldn't be impacted, as this is just compiler-hints etc, it is not a guarantee that there won't be any if used wrongly. Especially when disabling all optimization, some performance degradation is to be expected.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.