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

more missing includes #5930

Merged
merged 1 commit into from
Jul 1, 2024
Merged

more missing includes #5930

merged 1 commit into from
Jul 1, 2024

Conversation

schiele
Copy link
Contributor

@schiele schiele commented Jun 29, 2024

Here are more missing header files recently identified by building without precompiled header support.

@SoftFever
Copy link
Owner

I have some concerns. We might inadvertently "disable" the precompiling feature by adding more include header files.

Since the precompiled header is handled by the CMake module and Orca only supports the CMake build system, I wonder what is the scenario that Orca is compiled without precompiled headers?

@schiele
Copy link
Contributor Author

schiele commented Jun 30, 2024

I have some concerns. We might inadvertently "disable" the precompiling feature by adding more include header files.

Why do you fear disabling the precompiled header feature by adding the include statements properly? The way precompiled header support works is that a complete set of relevant header files gets precompiled in advance. The resulting precompiled header conglomerate gets fed into each individual translation unit before processing the translation unit itself. With that, all subsequent inclusions of header files already covered in the precompiled conglomerate get ignored. Therefore adding the needed header files to form well-formed and correct C/C++ files does not interfere with the feature.

Since the precompiled header is handled by the CMake module and Orca only supports the CMake build system, I wonder what is the scenario that Orca is compiled without precompiled headers?

I ran into this when using the makefile generator instead of the Ninja generator. I did so to more easily debug some issues when binding to some system libraries instead of those in the deps folder. The goal is to develop a system where I can choose for each individual dependency whether I prefer to take that from the system or from the deps folder. Once this effort provides some useful results I will for sure share those results.

Independent of my special use case I firmly believe that keeping code health on a high level in that regard comes with high value, in particular for two reasons:

  1. If you ever need to build the code on a compiler/system that does not come with precompiled header support or you don't want to use it, like getting better performance benefits by using tools like ccache it obviously does not work if translation units no longer properly include the needed header files and instead rely on some non-standard compiler mechanism to fix this up.
  2. If you rely on other independent translation units to include the header files for you, it can happen that when refactoring code in one translation unit and removing some no longer needed header includes from there you might break totally unrelated code that relied on those includes. Long-term this increases maintainability of the code.

I hope this helps to explain the idea behind those fixes. For sure those fixes are not needed to build the code as it is with the build instructions provided in the repository.

Here are more missing header files recently identified by building
without precompiled header support.
@SoftFever
Copy link
Owner

I hope this helps to explain the idea behind those fixes. For sure those fixes are not needed to build the code as it is with the build instructions provided in the repository.

You make a very good point.
I was under the wrong impression about the PCH.
After re-reading the Clang documentation, I realized the compiler ensures the PCH is processed before any "include" lines.

Copy link
Owner

@SoftFever SoftFever left a comment

Choose a reason for hiding this comment

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

LGTM
Thank you!

@SoftFever SoftFever merged commit ee67b8d into SoftFever:main Jul 1, 2024
12 checks passed
@schiele schiele deleted the missingincludes branch August 9, 2024 08: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.

2 participants