Skip to content
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

Closed
wants to merge 5 commits into from

Conversation

techgeeknz
Copy link
Contributor

This is split off from #8217; part 1 of 3.

JGRennison and others added 5 commits July 2, 2020 19:13
This will need fixups, because the commit checker won't let me indent
a nested #define.
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)
Copy link
Contributor

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?

Copy link
Contributor Author

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);
Copy link
Contributor

@nielsmh nielsmh Jul 2, 2020

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.

Copy link
Contributor Author

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 😉.

@LordAro
Copy link
Member

LordAro commented Jul 2, 2020

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

@techgeeknz
Copy link
Contributor Author

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?!

@techgeeknz techgeeknz closed this Jul 2, 2020
@LordAro
Copy link
Member

LordAro commented Jul 2, 2020

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.

@techgeeknz
Copy link
Contributor Author

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.

@techgeeknz techgeeknz deleted the jgr_master_gui_corefunc branch July 22, 2020 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants