💬 TheCharlatan commented on pull request "build: work around issue with Boost <= 1.80 and Clang >= 18":
(https://github.com/bitcoin/bitcoin/pull/30821#discussion_r1745569113)
Should this be in a `#if defined(__clang__)` block?

💬 theStack commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1774150203)
preprocessor nit, as one-liner:
```suggestion
#if defined(USE_BDB) && defined(USE_SQLITE)
```

💬 promag commented on pull request "qt6, test: Handle deprecated code":
(https://github.com/bitcoin-core/gui/pull/839#discussion_r1786676537)
2b5fd8fd24026ecf2fac7af6657e2363bfe3706c

An alternative to reduce duplicate code and the diff is to define `QVERIFY_EXCEPTION_THROWN`, something like?

```cpp
#if (QT_VERSION >= QT_VERSION_CHECK(6, 3, 0))
#define QVERIFY_EXCEPTION_THROWN(a, b) QVERIFY_THROWS_EXCEPTION(b, a)
#endif
```

💬 hebasto commented on pull request "qt6: Handle different signatures of `QANEF::nativeEventFilter`":
(https://github.com/bitcoin-core/gui/pull/840#issuecomment-2393637703)
> If Qt 6.7.0 is going to be required for Windows after the switchover; why change this code now just to immediately drop it after migrating to Qt 6?

To make https://github.com/bitcoin/bitcoin/pull/30997 focused on the build system changes only.

We already have:
```
$ git grep 'QT_VERSION_CHECK(6, 0, 0)'
src/qt/addressbookpage.cpp:#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
src/qt/addressbookpage.cpp:#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
src/qt/bitcoin.cpp:#if (QT_VERSION <
...

🤔 promag reviewed a pull request: "qt6: Handle different signatures of `QANEF::nativeEventFilter`"
(https://github.com/bitcoin-core/gui/pull/840#pullrequestreview-2349357223)
I think small changes to make code compatible with both versions are fine until support for qt5 is dropped.

Code review ACK 80761afced12c774a1768fb27a3975d456981ae0.

Alternative is to use type alias
```cpp
#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
using result_t = qintptr*;
#else
using result_t = long*;
#endif

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849132153)
Sorry, I forgot to include the diff which updates the macro (and 1 instance to show it compiles):

<details>
<summary>git diff on 6c9121f790</summary>

```diff
diff --git a/src/kernel/bitcoinkernel.h b/src/kernel/bitcoinkernel.h
index 9e6bf127db..67248349e2 100644
--- a/src/kernel/bitcoinkernel.h
+++ b/src/kernel/bitcoinkernel.h
@@ -31,9 +31,9 @@
#define BITCOINKERNEL_WARN_UNUSED_RESULT
#endif
#if !defined(BITCOINKERNEL_BUILD) && defined(__GNUC__) && BITCOINKERNEL_GNUC_PREREQ(3,
...

💬 hodlinator commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1880956056)
> https://godbolt.org/z/6336nGKcM (replace `0` with `1` to enable the compile-time checks)) into a compile failure on all compilers seems better than a runtime error, possibly only in CI.

With the `#if` set to `1` and lines 42-43 changed to
```C++
check = RuntimeFormat("fmt");
```
One still gets an ASAN error instead of a compile time error.

💬 hebasto commented on pull request "depends, NetBSD: Fix `bdb` package compilation with GCC-14":
(https://github.com/bitcoin/bitcoin/pull/31613#issuecomment-2590898164)
> It looks like `string.h` includes `strings.h` which defines these functions, but only if `_NETBSD_SOURCE` is defined....
>
> So agreed with @maflcko, let's just add a patch to include `strings.h` for netbsd.

The `strsep` function declaration is [guarded by `_NETBSD_SOURCE`](https://github.com/NetBSD/src/blob/c147cae3a562b7215a69630b1e3f3aaed09cd1dd/include/string.h#L97-L112) as well::
```c
#if defined(_NETBSD_SOURCE)
#include <strings.h> /* for backwards-compatibility */
__BEGIN_DEC
...

💬 brunoerg commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2721777570)
> Word of caution, i'm running in a nix-shell on a mac. So would like to see someone on pure mac env. confirm that this is the issue

Confirmed. I still have the issue on master but I just tested with the following diff and seems to work:

```diff
diff --git a/src/test/util/coverage.cpp b/src/test/util/coverage.cpp
index bbf068a6fa..ac0fb00e36 100644
--- a/src/test/util/coverage.cpp
+++ b/src/test/util/coverage.cpp
@@ -4,7 +4,7 @@

#include <test/util/coverage.h>

-#if defined(__cl
...

💬 Prabhat1308 commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2721800574)
> Word of caution, i'm running in a nix-shell on a mac. So would like to see someone on pure mac env. confirm that this is the issue

Applied this change and I get deterministic results on all runs
```
#if defined(__clang__) && (defined(__linux__) || defined(__APPLE__))
```
<img width="1512" alt="Screenshot 2025-03-13 at 9 31 40 PM" src="https://github.com/user-attachments/assets/fb622440-8634-47ba-b21a-54fc567803df" />

> Confirmed. I still have the issue on master but I just tested
...

💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2002955096)
I'm not exactly sure how we got to the `min_alloc` of `9` on these platforms, but we can probably simplify the platform dependent code to:
```C++
#if defined(__arm__) || SIZE_MAX == UINT64_MAX
constexpr size_t min_alloc{9};
#else
constexpr size_t min_alloc{0};
#endif
```

💬 hebasto commented on pull request "build: simplify *ifaddr handling":
(https://github.com/bitcoin/bitcoin/pull/32446#discussion_r2079675289)
"Define to 1 if '*ifaddrs' are available" implies that "it remains undefined otherwise". To achieve this, I suggest:
```suggestion
/* Define to 1 if '*ifaddrs' are available. */
#cmakedefine HAVE_IFADDRS 1
```

This also allows to replace the following `#if HAVE_IFADDRS` with `#ifdef HAVE_IFADDRS`.

💬 hebasto commented on pull request "test: Suppress Windows abort message in CI":
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2867088113)
As an alternative, we could explicitly set our own `SUPPRESS_ABORT_MESSAGE_BOX` environment variable in the workflow, and gate the code with this variable instead, removing the `#if defined(_MSC_VER)` condition.

💬 luke-jr commented on pull request "fs: use `ftruncate` in `AllocateFileRange` on OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/32645#discussion_r2132921035)
We're already calling fallocate here... seems like this whole #if mess should be sorted out.

💬 ajtowns commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3082011233)
CI failure seems to be due to [a bug in qt6 6.4](https://bugreports.qt.io/browse/QTBUG-31496?focusedId=888930&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-888930):

> Also for people coming here from Google: I had the same error message, but for me the problem was that I was using Qt 6.4.2 which apparently thinks that #if 202002L < 201103L is true, which causes c++config.h to not be included (and no #error is generated because moc doesn't support the directiv
...

💬 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")))
```

💬 maflcko commented on issue "OpenBSD, NetBSD: `-reindex` is broken":
(https://github.com/bitcoin/bitcoin/issues/33128#issuecomment-3206135519)
Diff to reproduce on Linux:


```diff
diff --git a/src/util/fs_helpers.cpp b/src/util/fs_helpers.cpp
index be7f1ee5a2..ada3280dbc 100644
--- a/src/util/fs_helpers.cpp
+++ b/src/util/fs_helpers.cpp
@@ -200,7 +200,7 @@ void AllocateFileRange(FILE* file, unsigned int offset, unsigned int length)
}
ftruncate(fileno(file), static_cast<off_t>(offset) + length);
#else
-#if defined(HAVE_POSIX_FALLOCATE)
+#if 0
// Version using posix_fallocate
off_t nEndPos = (off_t)offset + length;

...

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2336501679)
[C23 compiler support](https://en.cppreference.com/w/c/compiler_support/23.html) is still early and limited, but [typed enums](https://open-std.org/JTC1/SC22/WG14/www/docs/n3030.htm) would offer increased type safety to users that want it?

```C
/* C23-aware enum definitions */
#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 202311L && !defined(__cplusplus)
/**
* For C23 and later, use a strongly-typed enum. This creates a distinct type
* which is not implicitly convert
...

📝 fanquake opened a pull request: "depends: systemtap 5.3"
(https://github.com/bitcoin/bitcoin/pull/33373)
The diff in the copied header is:
```diff
< #if __STDC_VERSION__ >= 199901L
---
> #if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
```

From
https://sourceware.org/git/?p=systemtap.git;a=commit;h=b8345d8e07b725a943a97b19aa4866e74baadd98.

💬 hodlinator commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2387108889)
Could do potentially do something like...
```C++
#if defined(WAIT_FOR_DEBUGGER) && defined(NDEBUG)
#error "Should only enable WAIT_FOR_DEBUGGER in debug builds"
#endif
```
...though there could be scenarios where one wants to debug release builds as well.

*/contrib/guix/symbol-check.py* could also potentially analyze the binary after the fact. LLM says that the *lief* module can find calls to `prctl` when dynamically linking and otherwise one could add a dependency on the *capstone* mod
...