💬 maflcko commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856742649)
I looked at this and this is an upstream *boost* bug, not an llvm or bitcoin-core bug.
The reason is that the macro is a loop (not a do-while-0-loop):
```
#define BOOST_TEST_TOOL_IMPL( frwd_type, P, assertion_descr, TL, CT, ARGS ) \
do { \
BOOST_TEST_PASSPOINT(); \
::boost::test_tools::tt_detail:: \
BO
...
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856742649)
I looked at this and this is an upstream *boost* bug, not an llvm or bitcoin-core bug.
The reason is that the macro is a loop (not a do-while-0-loop):
```
#define BOOST_TEST_TOOL_IMPL( frwd_type, P, assertion_descr, TL, CT, ARGS ) \
do { \
BOOST_TEST_PASSPOINT(); \
::boost::test_tools::tt_detail:: \
BO
...
📝 0xB10C opened a pull request: "test: fix MIN macro redefinition"
(https://github.com/bitcoin/bitcoin/pull/31419)
Renames the `MIN` macro to `_TRACEPOINT_TEST_MIN`.
From #31418:
```
stderr:
/virtual/main.c:70:9: warning: 'MIN' macro redefined [-Wmacro-redefined]
70 | #define MIN(a,b) ({ __typeof__ (a) _a = (a); __typeof__ (b) _b = (b); _a < _b ? _a : _b; })
| ^
include/linux/minmax.h:329:9: note: previous definition is here
329 | #define MIN(a,b) __cmp(min,a,b)
| ^
1 warning generated.
```
fixes: https://github.com/bitcoin/bitcoin/issues/31418
(https://github.com/bitcoin/bitcoin/pull/31419)
Renames the `MIN` macro to `_TRACEPOINT_TEST_MIN`.
From #31418:
```
stderr:
/virtual/main.c:70:9: warning: 'MIN' macro redefined [-Wmacro-redefined]
70 | #define MIN(a,b) ({ __typeof__ (a) _a = (a); __typeof__ (b) _b = (b); _a < _b ? _a : _b; })
| ^
include/linux/minmax.h:329:9: note: previous definition is here
329 | #define MIN(a,b) __cmp(min,a,b)
| ^
1 warning generated.
```
fixes: https://github.com/bitcoin/bitcoin/issues/31418
💬 hodlinator commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#discussion_r1884641997)
This looks less weird, would it be equivalent in this case or defeat the purpose?
```suggestion
#define _FirstArg(args) _FirstArgImpl(args)
```
(https://github.com/bitcoin/bitcoin/pull/29256#discussion_r1884641997)
This looks less weird, would it be equivalent in this case or defeat the purpose?
```suggestion
#define _FirstArg(args) _FirstArgImpl(args)
```
💬 hodlinator commented on pull request "fuzz: Abort when using global PRNG without re-seed":
(https://github.com/bitcoin/bitcoin/pull/31486#discussion_r1887121592)
This is about excluding test-only code from production (currently fuzz-only since *check_globals.h/cpp* is in *fuzz/util/*). Not sure why that would be undesirable. Even though the conditional adds verbosity, it adds context and decreases need for comments.
Might be worth introducing a `TESTS_ENABLED` `#define`... but I'm guessing that has been discussed before?
(https://github.com/bitcoin/bitcoin/pull/31486#discussion_r1887121592)
This is about excluding test-only code from production (currently fuzz-only since *check_globals.h/cpp* is in *fuzz/util/*). Not sure why that would be undesirable. Even though the conditional adds verbosity, it adds context and decreases need for comments.
Might be worth introducing a `TESTS_ENABLED` `#define`... but I'm guessing that has been discussed before?
💬 maflcko commented on pull request "fuzz: Abort when using global PRNG without re-seed":
(https://github.com/bitcoin/bitcoin/pull/31486#discussion_r1888046321)
> Might be worth introducing a `TESTS_ENABLED` `#define`... but I'm guessing that has been discussed before?
I don't think it has. Maybe a separate issue or pull request can be created?
Whatever is done here also applies to the function immediately above, which is unrelated, so I'll leave this as-is for now.
(https://github.com/bitcoin/bitcoin/pull/31486#discussion_r1888046321)
> Might be worth introducing a `TESTS_ENABLED` `#define`... but I'm guessing that has been discussed before?
I don't think it has. Maybe a separate issue or pull request can be created?
Whatever is done here also applies to the function immediately above, which is unrelated, so I'll leave this as-is for now.
💬 hodlinator commented on pull request "fuzz: Abort when using global PRNG without re-seed":
(https://github.com/bitcoin/bitcoin/pull/31486#discussion_r1888106180)
Interesting that it hasn't really been discussed. There are trade-offs to adding it, but yeah, might experiment with adding it and create an issue.
Correct, `MakeRandDeterministicDANGEROUS()` should not exist unless `TESTS_ENABLED` was `#define`d (in that hypothetical scenario).
(https://github.com/bitcoin/bitcoin/pull/31486#discussion_r1888106180)
Interesting that it hasn't really been discussed. There are trade-offs to adding it, but yeah, might experiment with adding it and create an issue.
Correct, `MakeRandDeterministicDANGEROUS()` should not exist unless `TESTS_ENABLED` was `#define`d (in that hypothetical scenario).
💬 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.
(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.
💬 brunoerg commented on pull request "rpc: collect transaction fees on generateblock":
(https://github.com/bitcoin/bitcoin/pull/31775#discussion_r1938131196)
```
19:24:04.713] /ci_container_base/src/node/miner.cpp:150:9: error: 'exclusive_locks_required' attribute cannot be applied to a statement
[19:24:04.713] 150 | EXCLUSIVE_LOCKS_REQUIRED(m_mempool->cs);
[19:24:04.713] | ^ ~
[19:24:04.713] /ci_container_base/src/threadsafety.h:31:54: note: expanded from macro 'EXCLUSIVE_LOCKS_REQUIRED'
[19:24:04.713] 31 | #define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_requir
...
(https://github.com/bitcoin/bitcoin/pull/31775#discussion_r1938131196)
```
19:24:04.713] /ci_container_base/src/node/miner.cpp:150:9: error: 'exclusive_locks_required' attribute cannot be applied to a statement
[19:24:04.713] 150 | EXCLUSIVE_LOCKS_REQUIRED(m_mempool->cs);
[19:24:04.713] | ^ ~
[19:24:04.713] /ci_container_base/src/threadsafety.h:31:54: note: expanded from macro 'EXCLUSIVE_LOCKS_REQUIRED'
[19:24:04.713] 31 | #define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_requir
...
💬 luke-jr commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1970203305)
Just below here... the `#define` for close/open/fileno breaks using those names in other contexts (eg, C++ stdlib includes, potentially boost, whatever file includes this one, etc). At a minimum, we should probably define them as subprocess_(open|close|fileno) and define those to open/close/fileno for non-Windows too.
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1970203305)
Just below here... the `#define` for close/open/fileno breaks using those names in other contexts (eg, C++ stdlib includes, potentially boost, whatever file includes this one, etc). At a minimum, we should probably define them as subprocess_(open|close|fileno) and define those to open/close/fileno for non-Windows too.
💬 hodlinator commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#discussion_r1981191286)
nit: Less confusing for every branch in the block to result in `#define`:
```suggestion
#else
```
(https://github.com/bitcoin/bitcoin/pull/31158#discussion_r1981191286)
nit: Less confusing for every branch in the block to result in `#define`:
```suggestion
#else
```