Add missing includes and add simple test for includes in public headers #570
+35
−0
Conversation
This file contains 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
SaidBenaissa
changed the title
13 tasks
|
Hi! Thanks for this PR. We have added some fixes in the most recent release. We did a quick test and segyio build fine on Fedora. Do you think that your PR is still needed? |
|
I can confirm that the issue persist. I could reproduce it on Red Hat Linux 8. I will push some fixups and then try to get it merged soon. |
achaikou
approved these changes
Apr 4, 2025
achaikou
approved these changes
Apr 4, 2025
ajaust
force-pushed
the
fix_std_uint16_t_error
branch
3 times, most recently
from
7878718 to
92e2404
Compare
yngve793
approved these changes
Apr 9, 2025
achaikou
reviewed
Apr 9, 2025
|
That the tests sometimes pass without includes seems to be due to the used compilers and their standard libraries. Newer versions of GCC (I tested 13) seem to complain about missing |
- `algorithm` must be included because the header uses functions like `copy_n`. - `cstdint` must be included explicitly on Fedora-based Linux distribtuions, like Red Hat Enterprise Linux 8, to make all the used integer types available. - `stdexcept` must be included for definition of exceptions such as `runtime_error`.
These tests try to check whether the inclusion of segyio headers that are part of segyio's public interface prevent other projects from compiling. We had cases in which our public facing C and C++ headers did not define all required includes. This could lead to failures depending on the compiler and what other includes were and in which order when a segyio header was included. Testing for the correct headers to be included appears to be non-trivial and it is not 100% clear if this test is sufficiently accurate to prevent further problems in the future. At least the test tries to approximate how segyio's C/C++ interfaces are included in other projects such as the the si4ti project [1] that uses the C++ interface. If our tests is too simple, we could also try a tool like "include-what-you-use" [2]. If our test does not provide sufficient value, we can remove it again in the future. Note, the added test should fail at compile time if anything is wrong with the includes. [1]: https://github.com/equinor/si4ti [2]: https://github.com/include-what-you-use/include-what-you-use
ajaust
force-pushed
the
fix_std_uint16_t_error
branch
from
f5e4ade to
c35e43d
Compare
ajaust
changed the title
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.
No description provided.