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

@techgeeknz techgeeknz commented Jul 2, 2020

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

JGRennison and others added 5 commits Feb 25, 2020
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)

This comment has been minimized.

@nielsmh

nielsmh Jul 2, 2020
Contributor

You add these functions in the first commit, then change them in a later?

This comment has been minimized.

@techgeeknz

techgeeknz Jul 2, 2020
Author Contributor

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);

This comment has been minimized.

@nielsmh

nielsmh Jul 2, 2020
Contributor

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.

This comment has been minimized.

@techgeeknz

techgeeknz Jul 2, 2020
Author Contributor

I wish I'd known about that one earlier 😉.

@LordAro
Copy link
Member

@LordAro 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

@techgeeknz techgeeknz commented Jul 2, 2020

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

@techgeeknz techgeeknz commented Jul 2, 2020

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 techgeeknz:jgr_master_gui_corefunc branch Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.