💬 theuni commented on pull request "threading: remove ancient CRITICAL_SECTION macros":
(https://github.com/bitcoin/bitcoin/pull/32592#issuecomment-2919662378)
> https://cirrus-ci.com/task/6734127736553472?logs=ci#L3223
>
> ```shell
> [12:12:21.305] /ci_container_base/src/test/reverselock_tests.cpp:23:9: error: cannot call function 'AssertLockNotHeldInline' while mutex 'mutex' is held [-Werror,-Wthread-safety-analysis]
> [12:12:21.305] 23 | AssertLockNotHeld(mutex);
> [12:12:21.305] | ^
> [12:12:21.305] /ci_container_base/src/sync.h:141:31: note: expanded from macro 'AssertLockNotHeld'
> [12:12:21.305] 141 | #define A
...

💬 janb84 commented on pull request "cmake: Drop python dependency for translate":
(https://github.com/bitcoin/bitcoin/pull/33209#discussion_r2282214659)
Nit, Both define lines are a change in the `#ifdef` style (also from the previous generated file). Personally I do not care which style is followed. Just noting that this is a derivation.

```suggestion
#define UNUSED __attribute__((unused))
```

💬 edwargix commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2285678340)
please forgive me if I'm misunderstanding this, but can't this logic be simplified to:
```suggestion
#if defined(_WIN32)
#define BITCOINKERNEL_API __declspec(dllexport)
#elif defined(__GNUC__)
#define BITCOINKERNEL_API __attribute__((visibility("default")))
```

💬 davidgumberg commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2308685773)
Feel free to mark resolved, this suggestion is actually incorrect, since when `recv_result == 0` the `NLMSG_OK` check [below](https://github.com/bitcoin/bitcoin/blob/4c531782569b42fc47478a468f4a79e0e2d93946/src/common/netif.cpp#L110) would fail:


```c
#define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
(nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
(nlh)->nlmsg_len <= (len))
```
https://github.com/torvalds/linux/blob/39f90c1967215375f7d87b81d14b0f3ed6b40c
...

💬 ajtowns commented on issue "[29.x] guix build failure on ppc with xcb":
(https://github.com/bitcoin/bitcoin/issues/33488#issuecomment-3345222853)
Ah, there we go; these look like relevant lines:

```
$ HOSTS=powerpc64-linux-gnu contrib/guix/guix-build
...
xcb_auth.c:74:37: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
74 | #define AUTH_PROTO_MIT_MAGIC_COOKIE "MIT-MAGIC-COOKIE-1"
| ^~~~~~~~~~~~~~~~~~~~
xcb_auth.c:80:5: note: in expansion of macro 'AUTH_PROTO_MIT_MAGIC_COOKIE'
80 | AUTH_PROTO_MIT_MAGIC_COOKIE,
| ^~~~~~~~~~~~~
...

💬 hodlinator commented on pull request "refactor: replace `const char*` exceptions with `std::runtime_error`":
(https://github.com/bitcoin/bitcoin/pull/33569#discussion_r2412967418)
I don't get it, https://github.com/hebasto/bitcoin-core-nightly/actions/runs/18313967330/job/52151177201#step:6:401 logs:
`
test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" raised as expected: validation on the raised exception through predicate "HasReason{error}"
`

Looking at where the error message comes from - https://www.boost.org/doc/libs/1_87_0/boost/test/tools/old/interface.hpp, it has:
```C++
#define BOOST_CHECK_THROW_I
...

💬 ajtowns commented on pull request "refactor: Construct g_verify_flag_names on first use":
(https://github.com/bitcoin/bitcoin/pull/33600#discussion_r2427318037)
I had a look at this approach too. With C++20, I believe you can ensure the array is sorted at compile time with:

```c++
template<typename V, size_t N>
consteval auto SortedArray(const std::array<V,N>& pairs)
{
std::array<V,N> result = pairs;
std::sort(result.begin(), result.end());
return result;
}
#define FLAG_NAME(flag) std::pair<std::string_view, script_verify_flag_name>{#flag, SCRIPT_VERIFY_##flag}
constexpr auto g_verify_flag_names = SortedArray(std::array{
FLA
...

📝 maflcko opened a pull request: "util: Allow Assert() in contexts without __func__"
(https://github.com/bitcoin/bitcoin/pull/33785)
Without this, compile warnings could be hit about `__func__` being only valid inside functions.

```
warning: predefined identifier is only valid inside function [-Wpredefined-identifier-outside-function] note: expanded from macro Assert
115 | #define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
| ^
```

Ref https://github.com/bitcoin/bitcoin/pull/32740#discussion_r24862
...

🤔 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
...

💬 maflcko commented on pull request "util: Allow `Assert` (et al.) in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2509568255)
Just for ref, one could write this to please clang-format, but not sure if it is worth it:

```cpp
#define NONFATAL_UNREACHABLE() \
do { \
throw NonFatalCheckError{ \
"Unreachable code reached (non-fatal)", \
std::source_location::current(), \
}; \
} while (0)

💬 maflcko commented on pull request "build: add `-W*-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3522280482)
Not sure how to reproduce locally, but this seems to be failing:

```
/__w/bitcoin-core-nightly/bitcoin-core-nightly/src/clientversion.cpp:33:79: error: trailing whitespace [-Werror=trailing-whitespace=]
33 | //! git will put "#define GIT_COMMIT_ID ..." on the next line inside archives.
```

https://github.com/maflcko/bitcoin-core-nightly/actions/runs/19297432422/job/55182808314#step:9:403

💬 hebasto commented on pull request "build: add `-W*-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3522533346)
> Not sure how to reproduce locally, but this seems to be failing:
>
> ```
> /__w/bitcoin-core-nightly/bitcoin-core-nightly/src/clientversion.cpp:33:79: error: trailing whitespace [-Werror=trailing-whitespace=]
> 33 | //! git will put "#define GIT_COMMIT_ID ..." on the next line inside archives.
> ```
>
> https://github.com/maflcko/bitcoin-core-nightly/actions/runs/19297432422/job/55182808327#step:6:405

Does the following patch help:
```diff
--- a/src/clientversion.cpp
+++ b/sr
...

📝 maflcko opened a pull request: "refactor: Avoid -W*-whitespace in git archive"
(https://github.com/bitcoin/bitcoin/pull/33869)
Otherwise, compilation with GCC-15+ will warn about it:

```
src/clientversion.cpp:33:79: error: trailing whitespace [-Werror=trailing-whitespace=]
33 | //! git will put "#define GIT_COMMIT_ID ..." on the next line inside archives.
```

Follow-up to https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3522280482

Can be tested via `git archive --output=/tmp/a.tar HEAD`