Generally, compile-time checks should be close to the code that use them. Especially, since compat/assumptions.h is only included in one place, where iwyu suggests to remove it.
Fix all issues:
NDEBUG check is used in util/check, so it is redundant in compat/assumptions.h.__cplusplus check is redundant with doc/dependencies.md (see commit message).// IWYU pragma: keep to avoid removing the include by accident.
The check is already done in util/check.h, which is more widely
included.
It is unclear what the goal of this check is, given that the value may
need to be set lower for the mimimum supported version of compilers that
forgot to bump the value, see
https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1745143612 .
The minimum supported compiler versions are already documented in
doc/dependencies.md and using an older compiler will already result in a
compile failure, so this check can be removed as redundant. Especially
given that it is only included in one file, where iwyu suggests to
remove it.
Also add the <IWYU pragma: keep> to avoid removing it by accident.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | TheCharlatan, theuni, achow101 |
| Concept ACK | fanquake |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
std::endian by fanquake)If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Also see #20274 and #20274 (comment) particularly.
How would you enforce this? Include assumptions.h in every file where assert() is used? Not sure about that. A better approach might be to disallow assert() completely and force the use of Assert() (thus forcing the util/check include and compile-time check). However, that seems unrelated to the changes here.
Also, have you seen the commit message, which explains that util/check is included in more places than assumptions.h? You can check this via:
0$ git grep 'include <util/check.h' | wc -l
167
2$ git grep 'include <compat/assumptions.h' | wc -l
31
9@@ -10,7 +10,6 @@
10 #include <config/bitcoin-config.h>
11 #endif
12
13-#include <compat/assumptions.h>
14 #include <compat/compat.h>
15
16 #include <set>
Agree that this change moves the remaining assumption checks to where they are actually used (serialize.h). Checking the standard version again at compile time is kind of weird, especially if it is only done for files that include the assumption header.
I was thinking about this in the context of the kernel library currently not including the assumption header. Currently, a user just compiling the library would not get these compile-time checks, since the kernel does not use the one file that includes the assumption header. This PR improves on this current status.
De-duplicating the NDEBUG check between check.h and assumptions.h seem like a slight improvement to me. I don’t think leaving it in assumptions.h for documentation purposes is a compelling reason.
ACK 88887531b704f3943fdb33abbdd5378ecfeee14f
10@@ -11,13 +11,6 @@
11 #include <cstddef>
12 #include <limits>
13
14-// Assumption: We assume a C++17 (ISO/IEC 14882:2017) compiler (minimum requirement).
15-// Example(s): We assume the presence of C++17 features everywhere :-)
16-// ISO Standard C++17 [cpp.predefined]p1:
17-// "The name __cplusplus is defined to the value 201703L when compiling a C++
18-// translation unit."
19-static_assert(__cplusplus >= 201703L, "C++17 standard assumed");
I don’t see the harm in leaving this? It could be that it’s possible to compile certain parts of the code as c++14, but we want to document through code that c++17 is THE supported standard.
For ex, I have no idea, but it may currently be possible (or possible soon, after the next refactors) to build an app using libbitcoinkernel’s headers in c++14 mode. If that’s the case, we want to force that to be an error.
The harm is that no one knows what value to put here, see #28349 (comment) . Basically we’d have to go out and hand-pick the supported compilers and then ask them for their __cplusplus value and then put the minimum of all of them here. That seems backward.
I think a better approach would be to just write the code that the presumed supported compilers can compile correctly.
If there is a feature in a C++ version that we rely on explicitly in the code, I don’t think anything needs to be done, because either a compiler can compile the code or it can’t. For example, std::span. Using std::span and in the same file adding a static_assert that __cplusplus>=201709 (because GCC-10 sets that) doesn’t make sense. A bit better would be to use the feature test macro for std::span, which is __cpp_lib_span | 202002L. Though, I don’t see the benefit in that, over just using std::span directly.
If there is a feature in a C++ version that we rely on implicitly. For example, the epoch assumption in C++20 std::chrono, it could make sense to put the compile time assumption in our util/time.h header directly. (Either with a feature test macro or by using C++20 chrono code in the header)
__cpp_lib_span seems to work correctly: https://godbolt.org/z/4fsWE8WGf
To be clear, issues (sorta kinda) like this are the ones I’m trying to guard against here: #27930 .
In that case, the definition of things changed in c++20/23. Ideally we’d be able to select exactly one behavior we’re relying on (old or new) and fail to compile otherwise.
But yeah, I get that the state of things isn’t perfect, so I don’t care too much.
In that case, the definition of things changed in c++20/23. Ideally we’d be able to select exactly one behavior we’re relying on (old or new) and fail to compile otherwise.
Ok, I see, but then a check would need to be added for the upper bound, not the lower bound (which is removed in this pull). Currently there is no upper bound, so maybe it can be added in a separate/unrelated/parallel pull request?
Though, generally, we’ve been trying to also compile the codebase with the next major version of C++, so that any deprecation issues or other bugs would be caught early, ideally when new code is written. Thus, the code can be assumed to be (mildly) tested and working on the next version of C++ as well.
So I am not sure if adding an upper bound makes sense, because it would make testing minimally harder, such as the internal testing Microsoft did in #27930. (I understand they can just remove the one-line static assert, so no strong opinion).
6@@ -7,7 +7,10 @@
7 #define BITCOIN_SERIALIZE_H
8
9 #include <attributes.h>
10+#include <compat/assumptions.h> // IWYU pragma: keep
Concept ACK. We need to fixup/remove the __cplusplus so we can do C++20 while also supporting GCC 10.
Yeah, it would be a bit cleaner style-wise, if this was merged before C++20. However, it is not a blocker, because the check accepts anything greater than C++17, which should always pass for all supported compilers:
0static_assert(__cplusplus >= 201703L, "C++17 standard assumed");
