💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856612322)
> Seems easier to just fix it right away, instead of touching the line to say to fix it and then fix it in the next commit?

The commits have been squashed.

> But I'd like to understand the reason for the supressions: would the lint problem break CI without them?

Yes, it would.

💬 l0rinc commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#issuecomment-2498022212)
ACK f90fddfabd646c3f1d21de7bb004707a072e753e

💬 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'
```