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
Prepare: Update core function headers for refactor of GUI rendering #8255
Conversation
This will need fixups, because the commit checker won't let me indent a nested #define.
Hash mark must go in first column, as per https://wiki.openttd.org/Coding_style#Other_important_rules
Since they are only ever called with an int and a (int)uint; the templating seems completely unnecessary and redundant.
*/ | ||
template <typename S, typename U> | ||
static inline S DivTowardsNegativeInf(S a, U b) | ||
static inline int DivTowardsNegativeInf(const int a, const uint b) |
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.
You add these functions in the first commit, then change them in a later?
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.
Technically, @JGRennison added the functions and I changed them. Either way, I thought I'd squashed those together.
{ | ||
const int _b = static_cast<int>(b); | ||
const int b_ = static_cast<int>(b); |
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.
Why not just name this variable right in the commit adding it?
Also, an idea: Check if b >= std::numeric_limits<int>::max
and return 0 if it is. That avoids UB casting b to a type that can't fit the value, and will still have the correct result.
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 wish I'd known about that one earlier 😉.
Commits should be made up of individual logical changes. You're doing that, and that's good. But if you can reorder things so that commits are smaller or never necessary at all, that's even better. And again, you're adding functions that are then unused. These should be added (as a separate commit) in the PR that requires them |
This is splitting hairs. These are used in the follow-up PR; you asked me to split into smaller, easier-to-review chunks, and I've done that. Now you want them merged back together?! |
I want you to do relevant things at the same time. Adding functions in a separate set of changes and then not using them is pointless - it may be weeks or months before the main body of work is used (if indeed it ever ends up happening at all). The only thing I said should be split out is the Viewport renaming, which is otherwise irrelevant to the rest of the changes and causes a lot of "diff bloat" that makes the actual relevant changes harder to review. Splitting out the stdafx reformatting & adding sanity checks to existing functions is also recommended to be a separate PR - they are changes that aren't required for the main body of work. |
That is what I thought we were doing here; but never mind, we'll ride the merry-go-round again next week. |
This is split off from #8217; part 1 of 3.