-
Notifications
You must be signed in to change notification settings - Fork 50
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
Make each .cpp/.h file self-sufficient in #include
s
#72
Merged
Commits on Jun 22, 2024
-
Make each .cpp/.h file self-sufficient in
#include
sUntil now, some parts of the code used symbols from the standard library without including the corresponding standard library header using `#include` to ensure that the symbol is defined. That is, they silently assumed that the header has been already included by "someone else". These assumptions were sometimes met in our code due to transitive inclusions. For example, the missing `#include <stdexcept>` in `tests/unittest.cpp` would not turn into a compile error, because `tests/unittest.cpp` included `kaitai/exceptions.h`, which contains `#include <stdexcept>`. However, it is discouraged to rely on transitive inclusions, see https://google.github.io/styleguide/cppguide.html#Include_What_You_Use: > Do not rely on transitive inclusions. This allows people to remove > no-longer-needed `#include` statements from their headers without > breaking clients. This also applies to related headers - `foo.cc` > should include `bar.h` if it uses a symbol from it even if `foo.h` > includes `bar.h`. Instances of this problem in our code aren't limited to "maintainability issues" - they can also reduce portability. For instance, @roelschroeven apparently ran into a `strtoll not found` error in our `kaitai/kaitaistream.cpp` source file that he had to patch like this (see roelschroeven@db668fa): ```diff diff --git a/kaitai/kaitaistream.cpp b/kaitai/kaitaistream.cpp index a3810ab..4d5ded2 100644 --- a/kaitai/kaitaistream.cpp +++ b/kaitai/kaitaistream.cpp @@ -6,6 +6,7 @@ #include <iostream> #include <vector> #include <stdexcept> +#include <cstdlib> // ======================================================================== // Integer from raw data with correct endianness @@ -585,7 +586,7 @@ int64_t kaitai::kstream::string_to_int(const std::string& str, int base) { char *str_end; errno = 0; - int64_t res = strtoll(str.c_str(), &str_end, base); + int64_t res = std::strtoll(str.c_str(), &str_end, base); // Check for successful conversion and throw an exception if the entire string was not converted if (str_end != str.c_str() + str.size()) { ``` This makes sense according to https://en.cppreference.com/w/cpp/string/byte/strtol - we shouldn't assume that `std::strtoll` exists when we don't have `#include <cstdlib>` anywhere in our code. It worked fine without it in CI and on my local gcc 12.3.0 only because some standard library header that we include happened to use `<cstdlib>` as part of its implementation. For curiosity, I used this command locally to dump a tree that shows why each header was `#include`d: ``` c++ -DKS_STR_ENCODING_ICONV -DKS_ZLIB -I. -std=c++11 -H -MM ./kaitai/kaitaistream.cpp 2> kaitaistream-deps.log ``` So for example, we can see that on the gcc 12.3, the `<algorithm>` header includes `<cstdlib>` for some reason, so that's one of the reasons there's no compile error if we forget `#include <cstdlib>` in `kaitaistream.cpp`: ``` . /usr/include/c++/12/algorithm .. /usr/include/c++/12/bits/stl_algo.h ... /usr/include/c++/12/cstdlib ``` This is completely implementation-specific, of course. There's nothing in the C++ standard that requires `<algorithm>` to include `<cstdlib>`, and in other implementations this may not be the case at all. See also https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#sf10-avoid-dependencies-on-implicitly-included-names
Configuration menu - View commit details
-
Copy full SHA for 1175e8a - Browse repository at this point
Copy the full SHA 1175e8aView commit details -
Add
#include <cstddef>
, usestd::size_t
(not justsize_t
)We use `std::size_t` (and not the global `size_t`) because it is the only variant that `<cstddef>` is guaranteed to provide, see <https://developers.redhat.com/blog/2016/02/29/why-cstdlib-is-more-complicated-than-you-might-think>: > You must not assume that `<cxxx>` adds any names to the global > namespace, and you must not assume that `<xxx.h>` adds any names to > namespace `std`. So technically we could do it the other way around (`#include <stddef.h>` and use `size_t` everywhere), but the `<xxx.h>` headers are deprecated in C++ in favour of `<cxxx>`. Plus I think it's somewhat beneficial to use the `std::` prefix, because it reminds us that it's a type from the standard library, so it needs an `#include`.
Configuration menu - View commit details
-
Copy full SHA for a414d14 - Browse repository at this point
Copy the full SHA a414d14View commit details -
Add
KAITAI_STREAM_H_CPP11_SUPPORT
macro, fix C++98 compatibilitySee #72 (comment) The standard library header `<type_traits>` is only available since C++11 (see https://en.cppreference.com/w/cpp/header/type_traits), so we must not try to include it in C++98 mode.
Configuration menu - View commit details
-
Copy full SHA for c094e46 - Browse repository at this point
Copy the full SHA c094e46View commit details -
Configuration menu - View commit details
-
Copy full SHA for 6d422b2 - Browse repository at this point
Copy the full SHA 6d422b2View commit details -
Configuration menu - View commit details
-
Copy full SHA for 195c173 - Browse repository at this point
Copy the full SHA 195c173View commit details -
Configuration menu - View commit details
-
Copy full SHA for 748f5c5 - Browse repository at this point
Copy the full SHA 748f5c5View commit details -
Configuration menu - View commit details
-
Copy full SHA for c00507b - Browse repository at this point
Copy the full SHA c00507bView commit details -
Suppress IWYU warning about
#include <sys/param.h>
IWYU is actually right that `sys/param.h` is not used on Linux. However, we need it for the BSD detection, so let's tell IWYU that we want to keep this "unnecessary" inclusion.
Configuration menu - View commit details
-
Copy full SHA for 0fd74d4 - Browse repository at this point
Copy the full SHA 0fd74d4View commit details
Commits on Jun 30, 2024
-
Configuration menu - View commit details
-
Copy full SHA for 0373ae6 - Browse repository at this point
Copy the full SHA 0373ae6View commit details
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.