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

C++ Header Pass #19

Closed
3 tasks done
hollasch opened this issue Aug 25, 2019 · 7 comments
Closed
3 tasks done

C++ Header Pass #19

hollasch opened this issue Aug 25, 2019 · 7 comments
Assignees
Milestone

Comments

@hollasch
Copy link
Collaborator

hollasch commented Aug 25, 2019

GitHub user @p1v0t suggests using <cXXX> instead of <XXX.h> in our source code. Seems like a good idea, though they didn't include likely required fixes for uses of things now in the std:: namespace.

In general, it's a good idea, but we need to ensure that the existing codebase is either std:: clean or is made to be std:: clean.

In addition, includes should be have the associated header as the first include before anything else. For example, in foo.cpp, the header foo.h should be included before anything else. This ensures that any dependencies for the header itself have been declared without requiring additional includes first.

Then a blank line, then after that, local headers, in alphabetical order.

After that, separated by another blank line, all system and standard headers, in alpha order.

Thoughts?

If we agree to this, then it should be done for all three books:

  • In One Weekend
  • The Next Week
  • The Rest Of Your Life
@trevordblack
Copy link
Collaborator

I was kind of hoping that this wouldn't come up.

I don't agree that moving to c++ style headers is a good idea.

I'm open to being wrong here, but I've only ever seen moving from c headers to c++ headers as a "isn't broke, still going to fix" kind of thing.
C++ headers take longer to compile.
Are less translateable to other languages (since many languages can import c headers).
I'm personally not a fan of calling into std for everything.

I had a knee-jerk negative reaction to this, but I know I might be in the minority here.

@ronaldfw
Copy link
Contributor

I don't have an opinion on this other than that we should be consistent. (I realize my pull request for random number generation violates this by including .)

Since we generally seem to be moving in the direction of using more C++ features (including C++11), maybe C++ style headers would be more in line with that? I would think this is also cleaner from a naming perspective.

Regarding speed I would be interested to see any results, more out of curiosity than anything else.

But, to put this into perspective, both (global) naming issues and speed don't really matter for code at the scope of a textbook like this. It would be mostly beneficial from the perspective of setting a good example for folks who are learning.

Whatever we decide on, we may want to consider writing down decisions like this in a style guide.

@hollasch
Copy link
Collaborator Author

I lean toward modern C++, but can yield on that. The catch is that the original code uses cout instead of printf, and the current amalgam means readers need to have a passing understanding of both C and C++.

That said, the header organization I outlined above doesn't need to use modern headers, and it's a good start with sound reasoning behind the ordering.

We can leave the convert-all-headers to modern C++ for later.

@hollasch
Copy link
Collaborator Author

See original pull request (to the original InOneWeekend repo) — RayTracing/InOneWeekend#76

lorihollasch added a commit that referenced this issue Sep 2, 2019
lorihollasch added a commit that referenced this issue Sep 2, 2019
lorihollasch added a commit that referenced this issue Sep 2, 2019
@hollasch
Copy link
Collaborator Author

hollasch commented Sep 5, 2019

v2 work is now done — moving issue to post-v2.

@hollasch
Copy link
Collaborator Author

It looks like this would involve two headers:

  • float.h → cfloat
  • math.h → cmath

hollasch added a commit that referenced this issue Oct 27, 2019
- Got rid of float.h includes. Don't need these, since we're using
  infinity instead of FLT_MAX, defined in <limits>.
- <limits> included once in rtweekend.h.
- Include <cmath> once in rtweekend.h, vec3.h. Eliminates "math.h"
  everywhere else.

Resolves #19
hollasch added a commit that referenced this issue Oct 28, 2019
- Got rid of float.h includes. Don't need these, since we're using
  infinity instead of FLT_MAX, defined in <limits>.
- <limits> included once in rtweekend.h.
- Include <cmath> once in rtweekend.h, vec3.h. Eliminates "math.h"
  everywhere else.

Resolves #19
@hollasch
Copy link
Collaborator Author

Fixed in PR #242, development branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants