💬 maflcko commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856621868)
it would be good to understand why this is needed. If this is a bug, it would be good to fix it.

💬 brunoerg commented on pull request "fuzz: set the output argument of FuzzedSock::Accept()":
(https://github.com/bitcoin/bitcoin/pull/31316#issuecomment-2498034392)
So, it would be better to cherry-pick this into #28584?

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2498039344)
Can understand the checks failing on MacOS or Windows, but surprised that what I assume to be Linux on Cirrus CI is also failing. Will move to Windows and see how it behaves there. Might have to revert to explicitly checking `is_directory()`.

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856627172)
Right. I outlined this issue for follow-ups in the PR description.

💬 maflcko commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856627249)
See b32845cb94a81e6fd8b01a8631e3d276c9fc9e35, so I think this is intentional.

I think it would be better to fix the bug, instead of silencing it.

💬 l0rinc commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856631605)
We can likely simplify the call here, see: https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1846343700

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856640277)
Fixing all new "bugprone-use-after-move" warnings contradicts with other [comments](https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1846324884). On the other hand, fixing some of them is not consistent. That's why I chose to silence them.

`NOLINT` comments are easy searchable and effectively document the issues that have to be resolved.

👍 willcl-ark approved a pull request: "contrib: skip missing binaries in gen-manpages"
(https://github.com/bitcoin/bitcoin/pull/30986#pullrequestreview-2458506546)
ACK 4bbd28baf33382231f4f1dab20681c05f9915af2

Much better to add this behind a default-false flag

💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r1856659914)
2f9c2f4c6dcf8c45514e99d1018301aba61940fc: silent merge conflict with #30937.

💬 hebasto commented on pull request "refactor: Fix remaining clang-tidy performance-unnecessary-copy-initialization errors":
(https://github.com/bitcoin/bitcoin/pull/31364#issuecomment-2498088917)
https://cirrus-ci.com/task/4898976128827392:
```
/ci_container_base/src/qt/bitcoinunits.cpp:169:13: error: the variable 'whole' is copy-constructed from a const reference but is only used as const reference; consider making it a const reference [performance-unnecessary-copy-initialization,-warnings-as-errors]
169 | QString whole = parts[0];
| ^
| const &
```

💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r1856663445)
`PACKAGE_NAME` changed in #31042

💬 l0rinc commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856666679)
Unfortunately silencing will also invalidate the ACK-ed PR.
But if we could instead add `-bugprone-use-after-move` to `src/.clang-tidy` this concern could be addressed separately.

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856672277)
It will expose the whole codebase to re-introducing related bugs, no?

💬 l0rinc commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856679357)
We could tackle it in a separate pr like we do with the `performance-*` checks, right?

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856696985)
We could, but `performance-*` checks are not about safety, whereas `bugprone-*` checks are.

💬 l0rinc commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856700230)
If you decide to fix them here, I'll just rebase the related PR and ask for a re-review, it's not a tragedy :)

💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r1856703174)
Never mind, this causes more headaches that it solves.

💬 l0rinc commented on pull request "refactor: Fix remaining clang-tidy performance-unnecessary-copy-initialization errors":
(https://github.com/bitcoin/bitcoin/pull/31364#issuecomment-2498168179)
> the variable 'whole' is copy-constructed

Thanks, had to enable GUI as well (and clean the previous build directory, otherwise it's not detected at all for some reason):
```bash
git clean -fxd \
&& cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_GUI=ON \
&& cmake --build build -j$(nproc) \
&& run-clang-tidy -quiet -p build -j $(nproc) -checks='-*,performance-unnecessary-copy-initialization' | grep -v 'clang-tidy'
```

📝 TheCharlatan opened a pull request: "interpreter: Use the same type for SignatureHash in the definition"
(https://github.com/bitcoin/bitcoin/pull/31365)
This was missed during the original PR switching the nHashType argument to int32_t in SignatureHash in bc52cda1f3c007bdf1ed00aa3011e207c7531017.

The problem was discovered after running into a linker error when attempting to link this code as a static library using the header as a declaration with a riscv32 bare metal toolchain. The compiler would error with:
```
/opt/riscv-ilp32/lib/gcc/riscv32-unknown-elf/13.2.0/../../../../riscv32-unknown-elf/bin/ld: build_kernel_riscv/src/libbitcoin_con
...

💬 maflcko commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856742649)
I looked at this and this is an upstream *boost* bug, not an llvm or bitcoin-core bug.

The reason is that the macro is a loop (not a do-while-0-loop):

```
#define BOOST_TEST_TOOL_IMPL( frwd_type, P, assertion_descr, TL, CT, ARGS ) \
do { \
BOOST_TEST_PASSPOINT(); \
::boost::test_tools::tt_detail:: \
BO
...