Include fixups #4288
Merged
Include fixups #4288
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The current order of declarations and includes between "common.h" and "w32_crtdbg_stacktrace.h" is rather complicated. Both header files make use of things defined in the other one and are thus circularly dependent on each other. This makes it currently impossible to compile the "w32_crtdbg_stacktrace.c" file when including "common.h" inside of "w32_crtdbg_stacktrace.h". We can disentangle the mess by moving declaration of the inline crtdbg functions into the "w32_crtdbg_stacktrace.h" file and adding additional includes inside of it, such that all required functions are available to it. This allows us to break the dependency cycle.
17801f7 to
c9c2f76
Compare
The zlib dependency includes "common.h" inside of the "zconf.h" header to make available some type declarations like e.g. git_off_t. Including the "common.h" header does pull in quite a lot of other headers though, which are not required at all. Instead, we can just include our public "git2/types.h" header, which is much more limited in its scope but still provides everything required for "zconf.h". This fix eases the transition later on to use a separate "features.h" header instead of defines. As we have to generate the "features.h" header, we put it in the build directory and add an include directory. As we are splitting out building of dependencies into subdirectories, this would mean that the zlib dependency needs to be aware of the parent project's build directory, which is unfortunate. By including "git2/types.h", we avoid this problem.
Some of our header files are not included at all by any of their implementing counter-parts. Including them inside of these files leads to some compile errors mostly due to unknown types because of missing includes. But there's also one case where a declared function does not match the implementation's prototype. Fix all these errors by fixing up the prototype and adding missing includes. This is preparatory work for fixing up missing includes in the implementation files.
Some implementation files were missing the license headers. This commit adds them.
Next to including several files, our "common.h" header also declares various macros which are then used throughout the project. As such, we have to make sure to always include this file first in all implementation files. Otherwise, we might encounter problems or even silent behavioural differences due to macros or defines not being defined as they should be. So in fact, our header and implementation files should make sure to always include "common.h" first. This commit does so by establishing a common include pattern. Header files inside of "src" will now always include "common.h" as its first other file, separated by a newline from all the other includes to make it stand out as special. There are two cases for the implementation files. If they do have a matching header file, they will always include this one first, leading to "common.h" being transitively included as first file. If they do not have a matching header file, they instead include "common.h" as first file themselves. This fixes the outlined problems and will become our standard practice for header and source files inside of the "src/" from now on.
|
Seems reasonable to me. Thanks for this! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Up front: sorry for this big commit, I know it sucks. But as such I'm splitting this out from #4282 as it's too big to make it port of that one.
The first two commits fix up some small stuff regarding missing license headers and to fix some header files from not compiling which weren't included anywhere before. The next commit establishes a common include pattern across all our files in the "src/" folder. Quoting from its commit message:
"""
Make sure to always include "common.h" first
Next to including several files, our "common.h" header also declares
various macros which are then used throughout the project. As such, we
have to make sure to always include this file first in all
implementation files. Otherwise, we might encounter problems or even
silent behavioural differences due to macros or defines not being
defined as they should be. So in fact, our header and implementation
files should make sure to always include "common.h" first.
This commit does so by establishing a common include pattern. Header
files inside of "src" will now always include "common.h" as its first
other file, separated by a newline from all the other includes to make
it stand out as special. There are two cases for the implementation
files. If they do have a matching header file, they will always include
this one first, leading to "common.h" being transitively included as
first file. If they do not have a matching header file, they instead
include "common.h" as first file themselves.
This fixes the outlined problems and will become our standard practice
for header and source files inside of the "src/" from now on.
"""