💬 maflcko commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2449665430)
Then I think the only solution is to make it a build-time option again:

```cpp
constexpr bool g_fuzzing{
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
true
#else
false
#endif
};

💬 hodlinator commented on pull request "fuzz: Abort when using global PRNG without re-seed":
(https://github.com/bitcoin/bitcoin/pull/31486#discussion_r1886924031)
Skip in production
```suggestion
if constexpr (G_FUZZING) {
g_used_g_prng = true;
}
```
or use `#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`.

Maybe an optimizer would remove it as a dead-store, but better not to rely on that.

💬 hodlinator commented on pull request "qa debug: Add --debugnode/-waitfordebugger [DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/31723#issuecomment-2609977320)
> Ideally we wouldn't be adding debug code, to production binaries, to facilitate using an IDE.

Agree it should not be in production binaries, should be `#ifdef`ed away. Added as another reason for draft status.

> Can you configure your IDE to do something like the equivalent of using `gdb`/`lldb`, and `process attach --name bitcoind --waitfor`? So it just grabs bitcoind once it spins up?

I've never seen something like that in an IDE/editor, but I'll have a look, might exist a debugger
...

💬 hodlinator commented on pull request "qa debug: Add --debugnode/-waitfordebugger [DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/31723#issuecomment-2616686520)
Added tentative `#ifdef` solution in latest push. CMake part could probably be improved since now I think the `#define` leaks into the CMake projects public interface, which should not be necessary.

💬 fanquake commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#discussion_r2048555899)
I don't think we need to introduce checks here, we already use `clock_gettime`. You could just #ifdef `CLOCK_THREAD_CPUTIME_ID` at the callsite.

💬 fanquake commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#discussion_r2048557177)
I don't think we check for any other Windows functions like this, I think you can just #ifdef WIN32 at the callsite.

💬 l0rinc commented on pull request "feefrac: avoid integer overflow in temporary":
(https://github.com/bitcoin/bitcoin/pull/32300#discussion_r2049555771)
The simplest way I could find is to call it directly (not sure what the fuzzer did, don't have access):
```C++
BOOST_AUTO_TEST_CASE(feefrac_div_round_down_overflow)
{
#ifdef __SIZEOF_INT128__
constexpr auto divisor{2};
constexpr auto expected_result{std::numeric_limits<int64_t>::max()};
constexpr auto overflow_numerator{__int128{expected_result} * divisor + 1};
BOOST_CHECK_EQUAL(FeeFrac::Div(overflow_numerator, divisor, /*round_down=*/true), expected_result);
#else

...

💬 shahsb commented on pull request "fuzz: Suppress abort message on Windows":
(https://github.com/bitcoin/bitcoin/pull/32409#discussion_r2072293276)
```suggestion
#ifdef _WIN32
```

`_WIN32` is a compiler flag and could be used for both windows platform i.e windows_x86 (32-bit) as well as windows_x64 (64-bit)

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077565200)
nit: Wrap in `#ifdef WIN32`?

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

💬 fanquake commented on pull request "Use subprocess library for notifications":
(https://github.com/bitcoin/bitcoin/pull/32566#discussion_r2098299332)
I think you can just drop these `#ifdef`s around our own headers. Same for in `RunCommandParseJSON` below.

💬 Crypt-iQ commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2252321022)
Per offline discussion, removed the `#ifdef DEBUG_LOCKCONTENTION` since the category masks are now properly reset between iterations (thanks!).

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

💬 l0rinc commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2347458082)
If any of you feel strongly, I can change it, but it was a deliberate choice from my part.
See https://godbolt.org/z/o3sKqefT5 for an ugly error on Windows that also convinced me to just use the shorter version. I could of course hack it with:
```C++
#ifdef _WIN32
#include <windows.h>
#undef max
#else
```
or just hope that https://github.com/bitcoin/bitcoin/blob/9f744fffc39d93c9966dec1d61f113a7521983ad/CMakeLists.txt#L261 fixes it, but `SIZE_MAX` seemed simpler and shorter in this case :/

-
...

💬 hebasto commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2362554073)
This should work:
```suggestion
std::optional<size_t> GetTotalRAM()
{
auto clamp{[](uint64_t v) { return size_t(std::min(v, uint64_t{std::numeric_limits<size_t>::max()})); }};
#ifdef WIN32
if (MEMORYSTATUSEX m{}; (m.dwLength = sizeof(m), GlobalMemoryStatusEx(&m))) return clamp(m.ullTotalPhys);
#else
if (long p{sysconf(_SC_PHYS_PAGES)}, s{sysconf(_SC_PAGESIZE)}; p > 0 && s > 0) return clamp(1ULL * p * s);
#endif
return std::nullopt;
}

```

See CI outputs for:
- Fr
...

💬 vasild commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2362610449)
I came here for the same reason ;-) Here is the patch I devised:

```diff
std::optional<size_t> GetTotalRAM()
{
- auto clamp{[](uint64_t v) { return size_t(std::min(v, uint64_t{std::numeric_limits<size_t>::max()})); }};
+ [[maybe_unused]] auto clamp{[](uint64_t v) { return size_t(std::min(v, uint64_t{std::numeric_limits<size_t>::max()})); }};
#ifdef WIN32
if (MEMORYSTATUSEX m{}; (m.dwLength = sizeof(m), GlobalMemoryStatusEx(&m))) return clamp(m.ullTotalPhys);
-#elif define
...

🤔 hebasto reviewed a pull request: "system: silence unused variable warning and make GetTotalRAM() work on FreeBSD"
(https://github.com/bitcoin/bitcoin/pull/33435#pullrequestreview-3247240250)
Additionally, I suggest guarding this check:https://github.com/bitcoin/bitcoin/blob/56c6daa64f6bc29fb83f3ec8940c37ddc549edeb/src/test/system_tests.cpp#L23 with the same `#ifdef`s as in the `GetTotalRAM()` implementation. Otherwise, this test will fail on any unmentioned system.

🤔 hodlinator reviewed a pull request: "node: Add --debug_runs/-waitfordebugger + --debug_cmd"
(https://github.com/bitcoin/bitcoin/pull/31723#pullrequestreview-3274687166)
Thanks for reviewing @l0rinc!

> Since we're editing production code for this I would prefer having a test which checks that `support was not included as WAIT_FOR_DEBUGGER` is logged, proving that we're correctly excluding it from code (it's fine if the test isn't passing in debug mode)

Feel it would be a bit performative since `#ifdef`s and default-`OFF` CMake variables are pretty cut & dry. Maybe there's some scenario I haven't thought of.

💬 hodlinator commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2387995922)
I'm concerned with adding an extra build permutation for CI. Maybe could turn it on in some debug build. Hm..

Changed C++ code to use `constexpr` over `#ifdef` so that the code at least gets regularly compiled.

💬 l0rinc commented on pull request "random: clarify where the environ extern is needed":
(https://github.com/bitcoin/bitcoin/pull/33714#discussion_r2466889461)
If it's only needed on Mac, wouldn't:
```suggestion
#ifdef __APPLE__
```
be better here?