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