Make each .cpp/.h file self-sufficient in #includes
#72
Conversation
|
Very generally, I believe you're actually touching 2 connected, but separate problems here:
I don't mind either of these changes, but what I ideally want us to have is reproduction of any kind of problem in CI. I wonder if @roelschroeven can share details of their particular compilation which runs into problems, so we can add it here? |
See #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.
I agree. I don't think we can realistically get our hands on a set of compiler/platform combinations in CI that would be able to reliably detect whether our set of After installing it using pp@DESKTOP-89OPGF3:/mnt/c/temp/kaitai_struct/runtime/cpp_stl$ include-what-you-use -Xiwyu --verbose=3 -I. -DKS_STR_ENCODING_ICONV -DKS_ZLIB -std=c++11 -fPIC -Wall -Wextra -Wpedantic -Werror ./kaitai/kaitaistream.cpp
kaitai/kaitaistream.h should add these lines:
kaitai/kaitaistream.h should remove these lines:
- #include <istream> // lines 15-15
- #include <sstream> // lines 17-17
The full include-list for kaitai/kaitaistream.h:
#include <stdint.h> // for uint64_t, uint8_t, int64_t, int16_t, int32_t, uint16_t, uint32_t, int8_t
#include <ios> // for istream, istringstream, streamsize
#include <limits> // for numeric_limits
#include <string> // for string
#include <type_traits> // for enable_if, is_integral
---
kaitai/kaitaistream.cpp:553:42: warning: Bytef is defined in <zconf.h>, which isn't directly #included.
kaitai/kaitaistream.cpp should add these lines:
#include <zconf.h> // for Bytef
kaitai/kaitaistream.cpp should remove these lines:
- #include <cstddef> // lines 39-39
- #include <sstream> // lines 43-43
The full include-list for kaitai/kaitaistream.cpp:
#include <kaitai/kaitaistream.h>
#include <byteswap.h> // for bswap_32, bswap_64, bswap_16
#include <endian.h> // for __BYTE_ORDER, __BIG_ENDIAN, __LITTLE_ENDIAN
#include <iconv.h> // for iconv, iconv_close, iconv_open, iconv_t
#include <kaitai/exceptions.h> // for bytes_to_str_error, illegal_seq_in_encoding, unknown_encoding
#include <stdint.h> // for uint64_t, uint8_t, int64_t, uint32_t, int16_t, int32_t, uint16_t, int8_t
#include <zconf.h> // for Bytef
#include <zlib.h> // for z_stream, Z_NULL, Z_OK, inflate, inflateEnd, Z_STREAM_END, inflateInit
#include <algorithm> // for reverse
#include <cerrno> // for errno, EINVAL, E2BIG, EILSEQ, ERANGE
#include <cstdlib> // for size_t, strtoll
#include <ios> // for istream, operator|, ostringstream, basic_ios::clear, fpos, istringstream, streamsize, stringstream
#include <istream> // for basic_istream::read, operator<<, basic_istream::seekg, basic_istream::tellg, basic_istream::get, basic_istream<>::pos_type, basic_ostream, basic_ostream::operator<<, basic_istream::unget, basic_ostream<>::__ostream_type
#include <stdexcept> // for runtime_error, invalid_argument, out_of_range
#include <string> // for string, basic_string, getline, operator!=, basic_string<>::const_iterator, char_traits, char_traits<>::pos_type
#include <vector> // for vector
---
|
|
@GreyCat Please take another look. I added I only took the easy way to install it - https://packages.ubuntu.com/jammy/iwyu. That means we're using the version 0.17, which is somewhat dated (the latest version is 0.22 at the time of writing), but it's the latest version we can get from the Ubuntu package registry for our distribution, which is Ubuntu 22.04 "jammy". Installing a later version would be more complicated - I guess we'd have to build it from source. On top of that, since Clang 15 is the latest version we can get on "jammy", and 0.19 is the latest IWYU compatible with Clang 15, it wouldn't be much of an update (I mean it's better than 0.17, but still a few versions below 0.22) unless we figured out some way to install a newer Clang as well (it seems https://apt.llvm.org/ could help with that, but it obviously still requires some effort to make it all work).
|
|
I should note that IWYU suggests some changes in /home/runner/work/kaitai_struct_cpp_stl_runtime/kaitai_struct_cpp_stl_runtime/tests/unittest.cpp should add these lines:
#include <gtest/gtest-message.h> // for Message
#include <gtest/gtest-test-part.h> // for TestPartResult
#include "gtest/gtest_pred_impl.h" // for Test, SuiteApiResolver, EXPECT_EQ, TEST, TestFactoryImpl, CmpHelperFloatingPointEQ, FAIL, InitGoogleTest, RUN_ALL_TESTS, EXPECT_DOUBLE_EQ, EXPECT_FLOAT_EQ
/home/runner/work/kaitai_struct_cpp_stl_runtime/kaitai_struct_cpp_stl_runtime/tests/unittest.cpp should remove these lines:
- #include <gtest/gtest.h> // lines 4-4These changes are incorrect - according to GoogleTest docs, ... and the latest So let me also update to GoogleTest 1.14 to get rid of these incorrect suggestions. It requires building GoogleTest from source, but fortunately that's quite easy to do. |
See #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.
generalmimon
force-pushed
the
self-sufficient-includes
branch
from
ec91637
to
00015f8
Compare
Until 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
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`.
See #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.
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.
generalmimon
force-pushed
the
self-sufficient-includes
branch
from
00015f8
to
0fd74d4
Compare
|
@GreyCat This is now ready to merge - let me know if you have any objections. |
Until now, some parts of the code used symbols from the standard library without including the corresponding standard library header using
#includeto 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>intests/unittest.cppwould not turn into a compile error, becausetests/unittest.cppincludedkaitai/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: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 founderror in ourkaitai/kaitaistream.cppsource file that he had to patch like this (see roelschroeven@db668fa):This makes sense according to https://en.cppreference.com/w/cpp/string/byte/strtol - we shouldn't assume that
std::strtollexists 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#included: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>inkaitaistream.cpp: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