refactor: Remove redundant checks in compat/assumptions.h #28579

maflcko · 2023-10-04T09:29:42Z

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:

The NDEBUG check is used in util/check, so it is redundant in compat/assumptions.h.
The __cplusplus check is redundant with doc/dependencies.md (see commit message).
Add missing // 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 bitcoin#28349 (comment) . 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.

DrahtBot · 2023-10-04T09:29:46Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28674 ([POC] C++20 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.

hebasto · 2023-10-04T10:00:29Z

Also see #20274 and #20274 (comment) particularly.

maflcko · 2023-10-04T10:14:08Z

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:

$ git grep 'include <util/check.h' | wc -l
67
$ git grep 'include <compat/assumptions.h' | wc -l
1

TheCharlatan

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 8888753

src/common/system.h

TheCharlatan

re-ACK fa1a384

theuni

Concept ACK. Largely agree with @TheCharlatan, except I think the belt-and-suspenders std version check is fine to keep around.

theuni · 2023-10-16T18:45:32Z

src/compat/assumptions.h

-// ISO Standard C++17 [cpp.predefined]p1:
-// "The name __cplusplus is defined to the value 201703L when compiling a C++
-//  translation unit."
-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)

For reference, __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).

theuni · 2023-10-16T18:46:20Z

src/serialize.h

@@ -7,7 +7,10 @@
 #define BITCOIN_SERIALIZE_H

 #include <attributes.h>
+#include <compat/assumptions.h> // IWYU pragma: keep


The idea here is that this is basically our lowest-level header? Or because these assumptions mostly involve serialization?

Both: yes. I think anything that involves consensus/kernel would include this header. Also, the assumptions are used in serialize. (Also for block heights and in the wallet, ..., but all those include serialize as well).

fanquake · 2023-11-16T10:37:23Z

Concept ACK. We need to fixup/remove the __cplusplus so we can do C++20 while also supporting GCC 10.

maflcko · 2023-11-16T10:40:21Z

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:

static_assert(__cplusplus >= 201703L, "C++17 standard assumed");

maflcko · 2023-11-16T10:41:41Z

I was mostly waiting for a re-reply from @theuni and @hebasto. Though, given that they haven't replied in more than a month now, I presume they are fine with this pull in the current form?

theuni

ACK fa1a384

theuni · 2023-11-16T17:51:45Z

src/compat/assumptions.h

-// ISO Standard C++17 [cpp.predefined]p1:
-// "The name __cplusplus is defined to the value 201703L when compiling a C++
-//  translation unit."
-static_assert(__cplusplus >= 201703L, "C++17 standard assumed");


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.

achow101 · 2023-11-28T21:51:23Z

ACK fa1a384

MarcoFalke added 3 commits October 4, 2023 11:05

Remove duplicate NDEBUG check from compat/assumptions.h

faa3d4f

The check is already done in util/check.h, which is more widely included.

Move compat/assumptions.h include to one place that actually needs it

8888753

Also add the <IWYU pragma: keep> to avoid removing it by accident.

DrahtBot added the Refactoring label Oct 4, 2023

maflcko mentioned this pull request Oct 4, 2023

build: Require C++20 compiler #28349

Merged

DrahtBot mentioned this pull request Oct 4, 2023

Package Relay 1/3: Introduce TxDownloadManager and improve orphan-handling #28031

Closed

maflcko requested a review from theuni October 11, 2023 11:41

TheCharlatan approved these changes Oct 16, 2023

View reviewed changes

src/common/system.h Outdated Show resolved Hide resolved

Move compat.h include from system.h to system.cpp

fa1a384

maflcko force-pushed the 2310-assume- branch from fada516 to fa1a384 Compare October 16, 2023 12:34

DrahtBot added CI failed and removed CI failed labels Oct 16, 2023

TheCharlatan approved these changes Oct 16, 2023

View reviewed changes

theuni reviewed Oct 16, 2023

View reviewed changes

This was referenced Oct 17, 2023

refactor: [tidy] modernize-type-traits #28664

Closed

[POC] C++20 std::endian #28674

Closed

maflcko requested a review from theuni October 24, 2023 11:32

theuni approved these changes Nov 16, 2023

View reviewed changes

DrahtBot requested a review from fanquake November 16, 2023 17:53

achow101 merged commit 16b5b4b into bitcoin:master Nov 28, 2023

maflcko deleted the 2310-assume- branch November 28, 2023 22:00

bitcoin locked and limited conversation to collaborators Nov 27, 2024

refactor: Remove redundant checks in compat/assumptions.h #28579

refactor: Remove redundant checks in compat/assumptions.h #28579

Uh oh!

Conversation

maflcko commented Oct 4, 2023

Uh oh!

DrahtBot commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

hebasto commented Oct 4, 2023

Uh oh!

maflcko commented Oct 4, 2023

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanquake commented Nov 16, 2023

Uh oh!

maflcko commented Nov 16, 2023

Uh oh!

maflcko commented Nov 16, 2023

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Nov 28, 2023

Uh oh!

Uh oh!

DrahtBot commented Oct 4, 2023 •

edited

Loading