💬 stickies-v commented on pull request "refactor: C++20 operators":
(https://github.com/bitcoin/bitcoin/pull/33771#discussion_r2487080201)
nit (here and for all other new `operator<=>`): perhaps better to be explicit about the `std::strong_ordering` return type? And perhaps making it `constexpr` while touching?
<details>
<summary>git diff on 48840bfc2d</summary>
```diff
diff --git a/src/prevector.h b/src/prevector.h
index d4d90c7350..595be4a603 100644
--- a/src/prevector.h
+++ b/src/prevector.h
@@ -7,6 +7,7 @@
#include <algorithm>
#include <cassert>
+#include <compare>
#include <cstddef>
#include <cstdint>
...
(https://github.com/bitcoin/bitcoin/pull/33771#discussion_r2487080201)
nit (here and for all other new `operator<=>`): perhaps better to be explicit about the `std::strong_ordering` return type? And perhaps making it `constexpr` while touching?
<details>
<summary>git diff on 48840bfc2d</summary>
```diff
diff --git a/src/prevector.h b/src/prevector.h
index d4d90c7350..595be4a603 100644
--- a/src/prevector.h
+++ b/src/prevector.h
@@ -7,6 +7,7 @@
#include <algorithm>
#include <cassert>
+#include <compare>
#include <cstddef>
#include <cstdint>
...
💬 fanquake commented on pull request "ci, iwyu: Fix warnings in `src/kernel` and treat them as errors":
(https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3496969502)
> For reference, if devs want to copy the output, the job already has the full diff, ready to apply.
I don't think this diff is "ready to apply"? Headers like `errno.h` are wrong, and should be `<cerrno>` (this is enforced by `modernize-deprecated-headers` in the tidy job)? Includes like `#include "net_types.h"` are also wrong, and should be `#include <net_types.h>`?
Given that developers working on this code, have said that this change will likely make development more difficult, should
...
(https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3496969502)
> For reference, if devs want to copy the output, the job already has the full diff, ready to apply.
I don't think this diff is "ready to apply"? Headers like `errno.h` are wrong, and should be `<cerrno>` (this is enforced by `modernize-deprecated-headers` in the tidy job)? Includes like `#include "net_types.h"` are also wrong, and should be `#include <net_types.h>`?
Given that developers working on this code, have said that this change will likely make development more difficult, should
...
💬 fanquake commented on pull request "Changing the rpcbind argument being ignored to a pop up warning, inst…":
(https://github.com/bitcoin/bitcoin/pull/33813#issuecomment-3501963024)
https://github.com/bitcoin/bitcoin/actions/runs/19159736145/job/54788142845?pr=33813#step:5:171:
```bash
Duplicate include(s) in src/httpserver.cpp:
#include <node/interface_ui.h>
```
Can you also shorten the length of the commit title, and add a proper `prefix:`.
(https://github.com/bitcoin/bitcoin/pull/33813#issuecomment-3501963024)
https://github.com/bitcoin/bitcoin/actions/runs/19159736145/job/54788142845?pr=33813#step:5:171:
```bash
Duplicate include(s) in src/httpserver.cpp:
#include <node/interface_ui.h>
```
Can you also shorten the length of the commit title, and add a proper `prefix:`.
💬 maflcko commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2504767099)
Maybe:
```sh
echo "^^^ ⚠️ Failure generated from IWYU"
echo
echo "Some adjustments to the diff may be needed:"
echo
echo "* Use the C++ headers: E.g. <cerrno> over <errno.h>"
echo '* Use #include <__.h> over #include "__.h" for all headers'
echo "* Sort the headers, so that they are in the right pre-existing section, if"
echo " there are any."
(https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2504767099)
Maybe:
```sh
echo "^^^ ⚠️ Failure generated from IWYU"
echo
echo "Some adjustments to the diff may be needed:"
echo
echo "* Use the C++ headers: E.g. <cerrno> over <errno.h>"
echo '* Use #include <__.h> over #include "__.h" for all headers'
echo "* Sort the headers, so that they are in the right pre-existing section, if"
echo " there are any."
🤔 hebasto reviewed a pull request: "refactor: Add missing include in bitcoinkernel_wrapper.h"
(https://github.com/bitcoin/bitcoin/pull/33825#pullrequestreview-3438418410)
ACK fa1e8d8bad92f5fba2b086d78581df4c8123b098.
[Here](https://github.com/hebasto/bitcoin/commit/11bc5cca61a9edad38d070bd5046355fcf58c7ee#diff-4d05cd02fdce641be603f0f9abcecfeaf76944285d4539ba4bbc40337fa9bbc2) is IWYU's diff based on https://github.com/bitcoin/bitcoin/pull/33810:
```diff
--- a/src/kernel/bitcoinkernel_wrapper.h
+++ b/src/kernel/bitcoinkernel_wrapper.h
@@ -5,12 +5,19 @@
#ifndef BITCOIN_KERNEL_BITCOINKERNEL_WRAPPER_H
#define BITCOIN_KERNEL_BITCOINKERNEL_WRAPPER_H
-#include <kern
...
(https://github.com/bitcoin/bitcoin/pull/33825#pullrequestreview-3438418410)
ACK fa1e8d8bad92f5fba2b086d78581df4c8123b098.
[Here](https://github.com/hebasto/bitcoin/commit/11bc5cca61a9edad38d070bd5046355fcf58c7ee#diff-4d05cd02fdce641be603f0f9abcecfeaf76944285d4539ba4bbc40337fa9bbc2) is IWYU's diff based on https://github.com/bitcoin/bitcoin/pull/33810:
```diff
--- a/src/kernel/bitcoinkernel_wrapper.h
+++ b/src/kernel/bitcoinkernel_wrapper.h
@@ -5,12 +5,19 @@
#ifndef BITCOIN_KERNEL_BITCOINKERNEL_WRAPPER_H
#define BITCOIN_KERNEL_BITCOINKERNEL_WRAPPER_H
-#include <kern
...
📝 777genius opened a pull request: "refactor: Add missing include in util/result.h"
(https://github.com/bitcoin/bitcoin/pull/33832)
## Summary
Add missing `#include <type_traits>` for `std::conditional_t` and `std::is_same_v` used in the `Result` template class.
## Motivation
Following the same principle as #33825, headers should directly include standard library headers for types they use in their public interface, rather than relying on transitive includes.
Currently, `src/util/result.h` uses `std::conditional_t` and `std::is_same_v` (line 38) but does not directly include `<type_traits>`. While the code compiles due t
...
(https://github.com/bitcoin/bitcoin/pull/33832)
## Summary
Add missing `#include <type_traits>` for `std::conditional_t` and `std::is_same_v` used in the `Result` template class.
## Motivation
Following the same principle as #33825, headers should directly include standard library headers for types they use in their public interface, rather than relying on transitive includes.
Currently, `src/util/result.h` uses `std::conditional_t` and `std::is_same_v` (line 38) but does not directly include `<type_traits>`. While the code compiles due t
...
⚠️ hebasto opened an issue: "`test_kernel` fails to build on Ubuntu 22.04"
(https://github.com/bitcoin/bitcoin/issues/33846)
```
$ sudo apt update
$ sudo apt install git build-essential cmake pkgconf python3 libevent-dev libboost-dev libsqlite3-dev
$ git clone https://github.com/bitcoin/bitcoin.git && cd bitcoin
$ cmake -B build -DENABLE_IPC=OFF -DBUILD_KERNEL_LIB=ON
$ cmake --build build -j $(nproc)
<snip>
[ 76%] Building CXX object src/test/kernel/CMakeFiles/test_kernel.dir/test_kernel.cpp.o
/bitcoin/src/test/kernel/test_kernel.cpp:17:10: fatal error: format: No such file or directory
17 | #include <format>
...
(https://github.com/bitcoin/bitcoin/issues/33846)
```
$ sudo apt update
$ sudo apt install git build-essential cmake pkgconf python3 libevent-dev libboost-dev libsqlite3-dev
$ git clone https://github.com/bitcoin/bitcoin.git && cd bitcoin
$ cmake -B build -DENABLE_IPC=OFF -DBUILD_KERNEL_LIB=ON
$ cmake --build build -j $(nproc)
<snip>
[ 76%] Building CXX object src/test/kernel/CMakeFiles/test_kernel.dir/test_kernel.cpp.o
/bitcoin/src/test/kernel/test_kernel.cpp:17:10: fatal error: format: No such file or directory
17 | #include <format>
...
💬 maflcko commented on issue "`test_kernel` fails to build on Ubuntu 22.04":
(https://github.com/bitcoin/bitcoin/issues/33846#issuecomment-3515396269)
Please remove the include. See https://github.com/bitcoin/bitcoin/actions/runs/19176694315/job/54823183155?pr=33810#step:9:18532 :
```
/home/admin/actions-runner/_work/_temp/src/test/kernel/test_kernel.cpp should remove these lines:
- #include <boost/test/included/unit_test.hpp> // lines 9-9
- #include <cstdlib> // lines 15-15
- #include <format> // lines 17-17
(https://github.com/bitcoin/bitcoin/issues/33846#issuecomment-3515396269)
Please remove the include. See https://github.com/bitcoin/bitcoin/actions/runs/19176694315/job/54823183155?pr=33810#step:9:18532 :
```
/home/admin/actions-runner/_work/_temp/src/test/kernel/test_kernel.cpp should remove these lines:
- #include <boost/test/included/unit_test.hpp> // lines 9-9
- #include <cstdlib> // lines 15-15
- #include <format> // lines 17-17
💬 l0rinc commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2513989045)
do we still need `#include <pow.h>` here after the move?
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2513989045)
do we still need `#include <pow.h>` here after the move?
💬 ryanofsky commented on pull request "cmake: Move IPC tests to `ipc/test`":
(https://github.com/bitcoin/bitcoin/pull/33774#discussion_r2524524789)
In commit "cmake, test: Improve locality of `bitcoin_ipc_test` library description" (866bbb98fd365962840ee99df0d9f7ad557cc025)
This change should be correct, but I'm confused about how it was working before. This change causes includes in generated files such `build/src/ipc/capnp/common.capnp.proxy.h` to be generated correctly relative to `src/`:
```diff
-#include <src/ipc/capnp/common.capnp.h> // IWYU pragma: keep
+#include <capnp/common.capnp.h> // IWYU pragma: keep
```
So that see
...
(https://github.com/bitcoin/bitcoin/pull/33774#discussion_r2524524789)
In commit "cmake, test: Improve locality of `bitcoin_ipc_test` library description" (866bbb98fd365962840ee99df0d9f7ad557cc025)
This change should be correct, but I'm confused about how it was working before. This change causes includes in generated files such `build/src/ipc/capnp/common.capnp.proxy.h` to be generated correctly relative to `src/`:
```diff
-#include <src/ipc/capnp/common.capnp.h> // IWYU pragma: keep
+#include <capnp/common.capnp.h> // IWYU pragma: keep
```
So that see
...