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

Inline clean #813

Merged
merged 8 commits into from
Nov 24, 2020
Merged

Inline clean #813

merged 8 commits into from
Nov 24, 2020

Conversation

hollasch
Copy link
Collaborator

Despite comments in #803, we still had multiple cases where inline keywords were not required or where there was a better location for many of these global functions. Simple cleanup pass of different cases.

These are declared as static, so the `inline` keyword is unnecessary
as a definition-per-translation-unit modifier. Defer to the compiler for
a possible performance optimization.
Our quick and dirty illustration programs had a bunch of helper
functions in main.cc with the `inline` keyword. These are unnecessary.
This function was an inline global defined in `pdf.h`. Moved it to a
private method on class `sphere`, called only from `sphere::random()`.
There were multiple copies of the `random_cosine_direction()` utility
function in the cos_density.cc program and also in the `pdf.h` header.
Moved this function to `vec3.h` where it belongs.
@hollasch hollasch added this to the v4.0.0 milestone Nov 24, 2020
@hollasch hollasch requested review from trevordblack and a team November 24, 2020 07:06
@hollasch hollasch self-assigned this Nov 24, 2020
@trevordblack
Copy link
Collaborator

What's your plan with this PR here?

Move all member functions into the class definition,
and leave inline for any free functions?

@hollasch
Copy link
Collaborator Author

Yes, exactly. Trying to roll this out incrementally. I'm trying to avoid hairballs so it's easier to review.

Copy link
Collaborator

@trevordblack trevordblack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as a narrowly defined change, LGTM

@hollasch hollasch merged commit c645aee into dev-major Nov 24, 2020
@hollasch hollasch deleted the inline-clean branch November 24, 2020 22:47
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

2 participants